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

Broken code in shims #944

Closed
ChALkeR opened this issue Jul 10, 2020 · 4 comments
Closed

Broken code in shims #944

ChALkeR opened this issue Jul 10, 2020 · 4 comments
Labels
discussion Questions, feedback and general information.

Comments

@ChALkeR
Copy link

ChALkeR commented Jul 10, 2020

var PROMISE_ID = Math.random().toString(36).substring(16);

> Math.random().toString(36).substring(16).length
0

Also:

  • Why is that even Math.random()?
  • Why is that code even needed in that shim?
  • Why is that copy-pasted from some seemingly broken version instead of being a dep?
    See stefanpenner/es6-promise@375b0ea -- this was fixed 2.5 years ago upstream, apparently.
  • Why is the promise shim needed? Can it perhaps be just removed from the tree?

Those are all separate questions. ;-).

@ricmoo
Copy link
Member

ricmoo commented Jul 10, 2020

For many parts of those questions, “I don’t know”. ;)

That was the standard and popular Promise shim I found a long time ago.

I don’t recall all the issues surrounding it now, but it was an issue with phantomjs, the bundler and mocha with the test scripts that made it hard to pull in directly as a dependency. Or maybe it was what MDN linked to directly and I didn’t realize it was packaged on npm? :)

The shim is still required for several environments, I think. Certainly phantomjs. Possibly react or expo? If not, I’d love to remove it, since phantomjs is no longer maintained and it isn’t used for the test cases any more...

I can look back into this though. :)

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Jul 10, 2020
@ChALkeR
Copy link
Author

ChALkeR commented Jul 10, 2020

But phantomjs is dead (1, 2, 3). Is there much need in supporting that?

What is more important though: consumers can install their own shims from specialized well-supported packages (like es5-shim, es6-shim) if they need to.

Is there a reason to copy-paste those shims here?

@ricmoo
Copy link
Member

ricmoo commented Aug 26, 2020

Sorry, lost track of this. I'm working on the shims right now because react native is missing some pretty fundamental features... While fidgeting with that, I was reminded of this issue. :)

I know phantomjs is dead. The only reason to support it is that the v4 tests still use it, but I can manage its shims independently. Also, those browser tests are having other issues anyways.

I'm checking now to see what might break if I pull Promise out of the shims though. The main reason to provide it is because when I didn't lots of things broke. But 2016 were crazier times, and I think things seem better (less worse?) now.

At the time copy-paste made the most sense, for reasons I cannot remember, but I agree, I'd rather import them if they are still needed.

Anyhoo, overhauling the shims a bit right now... I'll tag this issue once I make the updates.

@ricmoo
Copy link
Member

ricmoo commented Sep 5, 2020

This file and shim has been entirely removed in 5.0.10.

Let me know if you find any other issues. :)

Thanks! :)

@ricmoo ricmoo closed this as completed Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Questions, feedback and general information.
Projects
None yet
Development

No branches or pull requests

2 participants