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

Transpile artifacts to es5 #199

Closed
wants to merge 5 commits into from

Conversation

git-jiby-me
Copy link

@git-jiby-me git-jiby-me commented Dec 7, 2018

There are no more traces of es6 syntax in the artifacts, which means given the consuming page has proper polyfills the library can be used on older browsers as well.

Artifact Before After
comlink.js 11.5KB 13.2KB
comlink.js gzip 3.4KB 3.7KB
messagechanneladapter.js 2.67KB 3.18KB
messagechanneladapter.js gzip 1.1KB 1.3KB

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@git-jiby-me
Copy link
Author

@surma FYI

@surma
Copy link
Collaborator

surma commented Dec 7, 2018

Hey @git-jiby-me, thanks for your hard work on this.

I’m torn. I see that there are developers who need to work with older browsers and can’t use Comlink out of the box as it is. I also see that transpiling to ES5 is suboptimal with the way Comlink is currently written. OTOH, I’m not willing to increase the code size for newer (more prominent) browsers and make it more complicated for new contributors to understand what’s happening in the code.

I think there’s a healthy middle ground here:

  • I’m 100% willing to give up the generator function. It’s just a gimmick, really.
  • I’m 100% not willing to give up arrow functions. These can easily be transpiled without significant overhead.
  • I’m undecided on async/await. I think making it easy to reason about the code and the RPC protocol is important, but I do see that the it adds transpilation overhead.

How about we only remove the generator for now and I’ll see if I can refactor Comlink to make it more ES5-transpilation-friendly? WDYT?

@git-jiby-me
Copy link
Author

@surma

I’m undecided on async/await. I think making it easy to reason about the code and the RPC protocol is important, but I do see that the it adds transpilation overhead.

The problem with leaving async await in code is that, tsc will then add the generator helper as well, resulting in almost the same overhead as if we transpile with no changes at all to the code IMO. I think we could only reach the middle ground you want probably by going the route that, we have two different set of bundles one for es6 and one for es5, but this comes with the overhead in development and testing that the tests needs to be done separately on both of these bundles, which personally i am not a fan of. So based on what i already know about what you want, these are our two options

  1. Keep es6 that requires helpers in code, but have a separate transpiled set of artifacts for es5
  2. Get rid of es6 that requires helpers in code, (this means async await as well)

I think 2 would be the best choice, but thats me personally, i can do another PR to try to split up the code and try to make it a bit more readable around Promises. Let me know what you think

@surma
Copy link
Collaborator

surma commented Mar 10, 2019

In the rewrite for v4, I am not using generators anymore, so this should hopefully solve this in a much more elegant way. Thank you for flagging this!

@surma surma closed this Mar 10, 2019
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.

None yet

3 participants