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

Error message patterns prevent Firefox from improving messages #55

Closed
miketaylr opened this Issue Oct 19, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@miketaylr
Copy link

miketaylr commented Oct 19, 2018

Over in Bug 1498257, we've had to back out some DX improvements to error messages from Firefox's JS engine because it broke Flipkart (the number 6 site in India, top 150 globally).

It turns out the reason is idx, see https://bugzilla.mozilla.org/show_bug.cgi?id=1498257#c7 for more details.

https://github.com/facebookincubator/idx/blob/master/packages/idx/src/idx.js#L73-L84

I'm not sure how widely this lib is used, but fixing here would allow us to attempt error message improvements again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c6e521429c is the changeset that was backed out. Some example improved error messages are:

TypeError: win.browser.runtime is undefined, can't access property "getManifest" of it
TypeError: [...][Symbol.iterator](...).next(...).value is null, can't access property Symbol.iterator of it

@miketaylr miketaylr changed the title Erorr message patterns prevent Firefox from improving messages Error message patterns prevent Firefox from improving messages Oct 19, 2018

@yungsters

This comment has been minimized.

Copy link
Contributor

yungsters commented Oct 19, 2018

In retrospect, I should never have implemented a runtime counterpart to idx (if even just for demonstration). It has enabled too many people to use it without the babel-plugin-idx. 👎

I'm sorry that this has prevented you from making progress. For now, the fastest path forward will probably be to add support for your new error messages here: https://github.com/facebookincubator/idx/blob/master/packages/idx/src/idx.js#L62

But if you have someone who you're working with at Flipkart, we should strongly encourage them to adopt babel-plugin-idx (which might also improve their runtime performance).

@davidben

This comment has been minimized.

Copy link

davidben commented Oct 19, 2018

Perhaps idx could wrap its first argument in a Proxy that does the undefined check on lookup, returning rewrapped values to keep going. Then you call the user-supplied function, and unwrap the result to return.

You may need to feature-detect support and fallback to error regexes, but it should leave newer browsers unconstrained.

@yungsters

This comment has been minimized.

Copy link
Contributor

yungsters commented Oct 20, 2018

Either suggestion seems fine to me. I would even be fine with changing idx to throw an error message stating that babel-plugin-idx has not been enabled.

But I probably won't be able to improve the polyfill anytime soon. I can help review pull requests, though.

@yungsters

This comment has been minimized.

Copy link
Contributor

yungsters commented Nov 16, 2018

@miketaylr, were you able to resolve the matter with Flipkart by using babel-plugin-idx?

@yungsters

This comment has been minimized.

Copy link
Contributor

yungsters commented Nov 24, 2018

Closing this out due to inactivity.

Feel free to open a new issue if you or anyone would like to revive the discussion about doing away with the runtime implementation.

@yungsters yungsters closed this Nov 24, 2018

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