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

Consider moving from fly-esnext to fly-mz #276

Closed
karimsa opened this issue Jun 11, 2017 · 11 comments
Closed

Consider moving from fly-esnext to fly-mz #276

karimsa opened this issue Jun 11, 2017 · 11 comments

Comments

@karimsa
Copy link

karimsa commented Jun 11, 2017

Fly currently has a hidden feature in which it attempts to pass the flyfile.js through fly-esnext for transpiling in order to add support for ES2015 features.

However, there's a number of issues with fly-esnext, the biggest one being inconsistent feature support due to the use of RegExp-based replacements. I know that it manages to have better performance than a parser-transpiler based version due to this but there have been many complaints of the lack of feature support (like how it fails with different spacing, variations of code, or that it runs the regexp engine 5 times in order to transpile the code - and the regexp engine is already so slow).

There are also certain other issues like the fact that the flyfile is run inside of a separate V8 sandbox and therefore does not have certain node features (like console is undefined).

However, it seems that the author of that project is insistent on keeping up the regexp way for the performance benefit. I wanted to propose the alternative to that package, a package which I just published called fly-mz. The API is exactly the same as what Fly expects from fly-esnext, but it uses babel-preset-env for the most optimal transpiling for the current version of node. It also fixes the sandbox issues.

I've currently added a hack to the postinstall script of fly-mz to use a symlink (fly-mz -> fly-esnext) in order to get Fly to work with the package.

@jorgebucaran
Copy link
Contributor

jorgebucaran commented Jun 11, 2017

@karimsa Fly currently has a hidden feature in which it attempts to pass the flyfile.js through fly-esnext for transpiling in order to add support for ES2015 features.

Yes, this is likely to change in the coming weeks.

However, it seems that the author of that project is insistent on keeping up the regexp way for the performance benefit.

Can you link to this issue or thread. I was not aware of this.

@karimsa
Copy link
Author

karimsa commented Jun 11, 2017

These issues mention the decision to stick to RegExp:

lukeed/fly-esnext#8 (comment)
lukeed/fly-esnext#5 (comment)

It seems that v1 of the esnext plugin was babel-based (via babel/register) and v2 moved to regexp.

@jorgebucaran
Copy link
Contributor

@karimsa Thanks for linking me to those!

@karimsa
Copy link
Author

karimsa commented Jun 11, 2017

No problem! Let me know what you think of the issue. If fly is definitely intending on supporting flyfile.js written in ESNext, another possibility is to integrate the fly-mz core directly into fly. It wouldn't take much - the module is fairly simple. That way there's no external dependency for that feature.

@jorgebucaran
Copy link
Contributor

@karimsa Thanks!

  1. We'll support esnext in the next version of fly, but won't rely on an external plugin for that.

  2. I want to start using more ES6-isms in the source as supported by node, but we need to be careful about not breaking compat with node 4 for a while. Perhaps we can update everything to node 7 or 8 and offer a fly-compat plugin to enable node 4 users, or just wait a bit more.

@karimsa
Copy link
Author

karimsa commented Jun 11, 2017

How would it work with a separate fly-compat plugin?

I personally see two possible solutions for switching over to using more ES2015 features:

  1. Update the minimum requirements for v3 and maintain LTS support for v2.
  2. Add a build process via fly (fly-inception) to transpile the code for node v4. This would add to the development dependencies, but it would still keep the dependencies at their current state. Through the env preset, it'll also ensure we transpile the minimum required.

By updating to the latest features, it would also be really nice to make use of async/await instead of coroutines - which can be wrapped via bluebird to maintain the performance.

@lukeed
Copy link
Owner

lukeed commented Jun 27, 2017

Hi @karimsa! Thanks a lot for this~!

Yes, v1 of the original esnext package did attach babel/register but I found that this greatly affected start-up and run-time performance. The boot-time is sort of a given, but Babel remains active/running for as long as Taskr remains running. Slows everything down & uses more system resources, despite the fact that Babel isn't doing anything anymore.

Since "lightweight" and "performance" are two of the pillars that makes Taskr stand out, this was a big no-no that had to change.

Along with this, I think it's important that we natively support what Node natively supports. This, again, harks back to those two pillars.

However!! That doesn't mean this would be a bad plugin! On the contrary! I think you'd be interested in my latest reply in #241 --- that would fit your esnext- alternative perfectly.

Lastly, I was going to support all import statements via RegExp. I've already done it elsewhere, just have to bring it into esnext 😄 Were there any other ES6-isms you were missing?

@jorgebucaran
Copy link
Contributor

jorgebucaran commented Jun 27, 2017

@lukeed You may also want to check this: https://github.com/karimsa/buildjs-benchmarks 🎉

EDIT: Grammar.

@lukeed
Copy link
Owner

lukeed commented Jun 27, 2017

Very cool! More evidence 😜

@karimsa If you don't mind, I can open a PR that will make us win the character count tests too 😆

@karimsa
Copy link
Author

karimsa commented Jun 27, 2017

Sounds good!

And yeah, absolutely. Feel free to PR any changes you feel make the tests more fair :)

@lukeed
Copy link
Owner

lukeed commented Jun 27, 2017

Great! If you wouldn't mind leaving a 👍 or comment on that thread, I'll appreciate/remember it the next time I look at that issue 😅

And okay 😃 I'm not sure if it's "fair" haha -- that'll be up for you to decide. Opening shortly~

Closing in favor of the other thread. Thanks!

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

No branches or pull requests

3 participants