-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Introduce async/await #829
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really happy about those changes :D Code look much more readable than previous 😸
I have only one comment: it looks that, changing dependency cause breaks backwards compatibility, please say me that I am wrong ;)
@alubbe hmm... What are we going to do with this pull request? I think it's a great idea to merge it but there is a lot of conflicts currently |
I've rebased the PR, fixed all conflicts and reverted to using the previous version of babel-polyfill, so that we're fully backwards compatible. Let me know what you guys think, I'd love to merge it! (And then we can start refactoring the tests to use async/await) |
Also, as I reported via my issue, travis no longer runs any tests - does anyone know why? |
Tests are green: https://travis-ci.org/exceljs/exceljs/builds/574842008 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alubbe Looks great!
I think it would good to get this in and then slowly dismantle the whole PromiseLib stuff.
Since it's likely to be a breaking change (for some) I think this warrants a new major version - what do you think?
I'd be fine with bumping the version, but in that case let's remove a lot of old leftovers:
So in short, it would be great to see this PR merged, and then let's spend a little time cleaning up more stuff, and then bump the version. |
Do we have anyone who feels comfortable merging the TS PRs? To me that's the only thing outstanding to merge this PR and get started on the next major version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@alubbe I've merged this and all the TS PRs I could see. Are there any I've missed? |
@alubbe @Siemienik @coldhiber The Main NPM artefact (what you get when you require/import 'exceljs' would come from the lib folder (we would probably want to bump engines directive to ">=8") To maintain support for those that still need ES5 in node we will babel into dist/exceljs.es5.js and dist/exceljs.es5.min.js Does anyone still use browserify ? If so we can keep them too but my thoughts are that modern web devs using react and angular (et al) can use the node type imports What do you think? |
Yes please! This was the main reason I introduced the Keeping the es5 build is nice, but we might also be fine completely dropping it, I don't think that developers that manipulate excel files via JS targeting old browsers will not be aware of transpilation. |
@alubbe I've done the refactor and tidied up all of the tests and pushed to master I think keeping the es5 still has value - it's still likely that there are builds out there where selectively including dependencies in the transpilation might be problematic. I feel the same is true with the browserify export. I know modern web dev is now overwhelmingy react, angular (and similar) and looking at google trends comparing webpack with browserify mirrors this, but once again - there will be builds out there still relying on the browserify export. Given that there's practically next to no cost in keeping them, I think we might as well I'm going to leave these changes in master for a bit, I'd like to test it out as a github dependency to check it's all ok... |
Just saw that you released 3.0.0, looks great - let's wait for feedback from usage in the real world ;) |
Here it comes! 🎉
In this PR, I've not modified any tests so that we know that these changes are a pure refactoring.
Unfortunately,
async
uses the internalPromise
object and we cannot offer our users to use a different Promise library. To be backwards compatible with promish, I've added babel polyfill, but I would expect that most consumers of this library will start to use the modern entrypoints.Looking forward to your feedback!