Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail gracefully in unsupported browsers #18

Closed
SystemParadox opened this issue Aug 6, 2015 · 15 comments
Closed

Fail gracefully in unsupported browsers #18

SystemParadox opened this issue Aug 6, 2015 · 15 comments
Assignees

Comments

@SystemParadox
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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 dougwilson self-assigned this Sep 6, 2015
@dougwilson
Copy link
Owner

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
Copy link
Author

SystemParadox commented Sep 6, 2015 via email

@dougwilson
Copy link
Owner

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
Copy link
Owner

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 added a commit that referenced this issue Sep 7, 2015
dougwilson added a commit that referenced this issue Sep 7, 2015
@dougwilson
Copy link
Owner

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

@SystemParadox
Copy link
Author

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

@SystemParadox
Copy link
Author

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants