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

Remove Promise polyfill #1758

Closed
wants to merge 1 commit into from

Conversation

rolandjitsu
Copy link

This addresses #955. It basically just removes es6-promise polyfill as there is really no need for it unless you use a legacy browser.

I've also added some doc on this matter for users that do still use legacy browsers.

@rolandjitsu
Copy link
Author

Note that tests were failing before I made any changes.

@etpinard
Copy link
Contributor

etpinard commented Jun 5, 2017

Thanks for the PR. But, we can't just do that.

Dropping support for Promise-less browsers will have to wait until v2. You'd be surprised to know how many plotly.js users still need support for IE11. So, I'll have to close this PR until we get serious about making a push for v2.

Like I said in #955 (comment), we could publish a a dist/ bundle without the Promise-polyfill (if you're up for a challenge, that's a PR we'd accept). Alternatively, we could add an option to include or not include a Promise polyfill in #768.

@etpinard etpinard closed this Jun 5, 2017
@rolandjitsu
Copy link
Author

@etpinard I understand, but it's been a while since #955 has been opened and I thought I should give it a try and remove the polyfill.

As I explained in the issue, users should not be forced to use a polyfill if they don't want to and it should be up to them to provide the necessary APIs that plotly would require.

Anyhow, I also understand that breaking compatibility is not an option. So making an extra bundle does not sound like a bad idea. How should I approach that, because I'm not familiar with the build system for this lib?

@KazimirPodolski
Copy link

@etpinard why not just avoid polluting of global scope with your polyfill? I understand you want promises and you need polyfill, I've just wondering why you made graphing library export anything but graphing.

@etpinard
Copy link
Contributor

etpinard commented Jun 8, 2017

why not just avoid polluting of global scope with your polyfill?

That's a good idea. That might still be considered a breaking change, but it's worth investigating. Anyone down to submit a PR?

I've just wondering why you made graphing library export anything but graphing.

People make mistakes. Sorry.


I should add:

  • plotly.js v1.x need to just work in IE11 (i.e. one <script> tag and no third-party polyfill).
  • plotly.js SVG trace types also need to work in IE9 and IE10 with typedarray and rAF polyfills.
  • plotly.js v2.x will use the browsers' native window.Promise (i.e. IE users will need a Promise polyfill).

@rolandjitsu
Copy link
Author

@etpinard I'd give it a shot at making another bundle without polyfills, but I need to understand how the bundles are created.

@etpinard
Copy link
Contributor

etpinard commented Jun 8, 2017

It starts here: https://github.com/plotly/plotly.js/blob/master/tasks/bundle.js

Learning a few things about the browserify api first might help.

@KazimirPodolski
Copy link

People make mistakes. Sorry.

Understood. I've rebuilt it as stated in #955. Sorry I can't help with PR - I'm not familiar with Plotly code.

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

Successfully merging this pull request may close these issues.

3 participants