Skip to content

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

Merged
merged 4 commits into from
Sep 2, 2019
Merged

Introduce async/await #829

merged 4 commits into from
Sep 2, 2019

Conversation

alubbe
Copy link
Member

@alubbe alubbe commented May 22, 2019

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 internal Promise 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!

Copy link
Member

@Siemienik Siemienik left a 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 ;)

@Siemienik
Copy link
Member

@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

@alubbe
Copy link
Member Author

alubbe commented Aug 21, 2019

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)

@alubbe
Copy link
Member Author

alubbe commented Aug 21, 2019

Also, as I reported via my issue, travis no longer runs any tests - does anyone know why?
You'll have to check out this branch locally and run the tests there to verify that everything is green (it is on my machine)

@alubbe
Copy link
Member Author

alubbe commented Aug 23, 2019

@alubbe alubbe mentioned this pull request Aug 23, 2019
Copy link
Collaborator

@guyonroche guyonroche left a 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?

@alubbe
Copy link
Member Author

alubbe commented Aug 23, 2019

I'd be fine with bumping the version, but in that case let's remove a lot of old leftovers:

  • Bump @babel-polyfill version
  • Replace the current entry with their ES6 version
  • ... other stuff?

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.
So maybe let's merge the TypeScript PRs, bump v1 and then merge this PR?

@alubbe
Copy link
Member Author

alubbe commented Aug 28, 2019

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.

Copy link
Contributor

@defshift defshift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@guyonroche guyonroche merged commit 38a085c into exceljs:master Sep 2, 2019
@guyonroche
Copy link
Collaborator

@alubbe I've merged this and all the TS PRs I could see. Are there any I've missed?
I'd like to clean up the whole PromiseLib mess before bumping the version then we can move forwards from there :-)

@alubbe alubbe deleted the async-await branch September 2, 2019 08:31
@guyonroche
Copy link
Collaborator

@alubbe @Siemienik @coldhiber
Hi Guys - I'm thinking it might be time for yet another major change - this time to refactor the published artefacts...

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?

@alubbe
Copy link
Member Author

alubbe commented Sep 25, 2019

Yes please! This was the main reason I introduced the modern files and I agree, they should be the default now.

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.

@guyonroche
Copy link
Collaborator

@alubbe I've done the refactor and tidied up all of the tests and pushed to master
"verquire" in the specs is promoted to a global now and will switch on the EXCEL_BUILD env to pull in lib/ or dist/es5/ code.

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...

@alubbe
Copy link
Member Author

alubbe commented Oct 2, 2019

Just saw that you released 3.0.0, looks great - let's wait for feedback from usage in the real world ;)

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.

4 participants