dougwilson / nodejs-depd

Deprecate all the things
MIT License
325 stars 87 forks source link

Fail gracefully in unsupported browsers #18

Closed SystemParadox closed 9 years ago

SystemParadox commented 9 years ago

Until #16 is implemented, please please can we at least have graceful failure when running in unsupported browsers?

I don't mind whether you want to print a warning every time, never print a warning, or use console.error, but I would like the rest of my code to run. At the moment depd blows up with an exception and everything stops :(

In reference to #14, defining a fallback isn't a good solution at all:

  1. It works fine in Chrome - a fallback would prevent this
  2. It's not my module's responsibility - depd does non-standard things, not me
  3. There is no compatible fallback module available

Thanks.

dougwilson commented 9 years ago

Hi! What environment are you looking to have a fallback in? Would you be willing to make a PR? If not a PR, then please let me know enough information so I can make the appropriate code :)

SystemParadox commented 9 years ago

As per #14, in Firefox it throws this error:

TypeError: Error.captureStackTrace is not a function

I will gladly make a PR, but wasn't sure exactly what I should be checking against. Is it sufficient just to check for Error.captureStackTrace?

I am yet to come across an environment which doesn't have a stack property on error objects. Is there any reason why we can't just use this? I am assuming that the (V8 specific?) Error.prepareStackTrace, Error.captureStackTrace, Error.stackTraceLimit are as some kind of performance optimisation?

dougwilson commented 9 years ago

Hi! Can you say an exact version of Firefox I need to target as well? For example, do I need to ensure it worked on Firefox 3.5 or is some other version more appropriate?

The stack property is a string. Different environments format their string differently and the format is not standard. This means you're just going to have the same incompatibilities with the stack property.

We actually interrogate the stack at a level that the stack string just really does not work. The stack string also makes it virtually impossible to use if a file name contains a new line character (yes, file names can contain a new line, so you cannot just split the stack on "\n").

I hope that helps explain why this module can not use the string stack property.

dougwilson commented 9 years ago

Oh, and yet another issue is that the .stack property only contains a portion of the stack. This means if what we are looking for is too far on the stack, it won't even be contained in the .stack property.

Essentially this module just cannot function at all without the V8 stack functions (or something similar). What makes this module great is that it will include the exact callsite of the usage in your code on warning, it will not warn on even usage (think about a webserver that uses a deprecated method for a request--that would mean one message per request!), it will also ensure that you are warned for each unique usage in your code, not just stop warning after the first use. This allows people to run a test suite once and get all the warning they need to fix, not run once, get one warning, fix, run again, get second warning, fix, etc.

dougwilson commented 9 years ago

2.It's not my module's responsibility - depd does non-standard things, not me

Also, on this point, it actually is not our responsibility, as you can see from the package.json, we have clearly outlined out supported environments. Our contract with users is this code is only to be used in Node.js 0.6+; any other use is at you own risk.

dougwilson commented 9 years ago

Also, it may be useful another user attempted this before in #15 but after they dug into the code, abandoned the PR. Just pointing you to the PR so essentially you can read it and pick up where it was left off :)

SystemParadox commented 9 years ago

I'm using Firefox 35.0, any reasonably recent version would be fine.

Yes I love the single-warning and proper callsite, it makes my life so much easier :)

Thanks for the info on .stack, that's worth knowing. I guess we just need to check for the availability of V8 functions then.

dougwilson commented 9 years ago

Hi @SystemParadox , I'm looking into this, and have another question for you: how are you getting this code into the web browser in the first place? Are you just copy-pasting the code, or using a system like browserify or webpack (and if so, which one specifically)?

SystemParadox commented 9 years ago

I am using browserify.

dougwilson commented 9 years ago

Awesome, thanks, @SystemParadox ! So what I'm going to do for now, while #16 is underway, is get this module to just work in a "noop" mode when in the browser. I was thinking of doing this simply with package.json tweak for now, which would work with browserify/webpack. I think that this will get modules using this one to work in the meantime, while I work on replicating as much functionality as I can for web browsers, at which time those module can take advantage of when it's released.

How does that sound :) ?

dougwilson commented 9 years ago

Ok, master has been updated to add a different branch (for now) when bundling with Browserify for the web environment. Let me know if it works for you and I'll release it out there :)

dougwilson commented 9 years ago

Hi @SystemParadox , any feedback on the current master implementation?

SystemParadox commented 9 years ago

Sorry I'm away at the moment, will have a look at this next week.

SystemParadox commented 9 years ago

I can remove my shim and it still runs in Firefox :)

However, in the longer term I don't think you need to use a package.json shim. Just check for the availability of the V8-specific APIs (Error.captureStackTrace, etc) before you use them. If they are not available, then noop or fallback to alternative implementation. It worked fine in Chrome before, by using a package.json shim we've lost that.

dougwilson commented 9 years ago

I understand and this is only the short-term fix so that people's packages run in Firefox at all. I am working on a more robust one.