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

Change how the compat builds are produced #269

Closed
stasm opened this issue Aug 9, 2018 · 19 comments
Closed

Change how the compat builds are produced #269

stasm opened this issue Aug 9, 2018 · 19 comments

Comments

@stasm
Copy link
Contributor

stasm commented Aug 9, 2018

#133 changed the browser compatibility matrix of our compat builds. It was a good decision for Fluent and for the web. Unfortunately, while most browser support modern JavaScript, it turns out a significant portion of build tools like Webpack and more specifically, UglifyJS, do not 🤦‍♂️ .

I'm afraid we might need to re-implement strict ES5 compatibility in one way or another. I don't think we can expect existing projects to go through the whole ordeal of updating their build pipeline just to be able to use a newer version of fluent and friends.

Right now we produce two builds for each package:

  • index - no transpilation at all, and
  • compat - transpiled according to the modern compatibility matrix.

The moder compatiblity matrix expects the engine to support async functions, classes and generators, among others.

I propose we switch to the following strategy:

  • index - transpiled according to the modern compatibility matrix,
  • compat - transpiled to strictly ES5-compatible code,
  • vanilla - not transpiled at all. This could also be called modern or ecma.

The vanilla builds will be allowed to use cutting-edge features like async iteration, implemented only in Firefox 57 and Chrome 63.

@stasm
Copy link
Contributor Author

stasm commented Aug 9, 2018

@Pike, @zbraniecki - go / no go?

@Pike
Copy link
Contributor

Pike commented Aug 9, 2018

index being fluent/fluent.js, right?

What I'd like us to have is some testing infrastructure that tells us if we're going to run into trouble while we're working on PRs. Asking a couple of stupid questions, because I know Jack about how JS works.

Is the ES5-compatible transpilation going to tell us that? Can we run tests against it? Track its size?

Also, do those entry points publish code we generate, or are they instructing other folks on how to generate it, and could they fail because of their own setup?

@stasm
Copy link
Contributor Author

stasm commented Aug 9, 2018

index being fluent/fluent.js, right?

Yes. I've used index because it's accessible as import ... from "fluent" rather than from "fluent/something". I also think I'd change fluent/fluent.js to fluent/index.js and the same for all other packages as part of this change. index is usually how the main entry point is called in node-world.

Is the ES5-compatible transpilation going to tell us that? Can we run tests against it? Track its size?

We could have a test which tries to run the ES5-compatible version and makes sure all known required polyfills are accounted for. Does this answer your question?

Also, do those entry points publish code we generate, or are they instructing other folks on how to generate it, and could they fail because of their own setup?

I think we should publish all of them. Cosumers of the library will be then able to choose any of:

  • import {MessageContext} from "fluent";
  • import {MessageContext} from "fluent/compat";
  • import {MessageContext} from "fluent/vanilla";

@Pike
Copy link
Contributor

Pike commented Aug 9, 2018

Would it help to run a couple of integration tests on each bundle in a couple of node versions?

(couple in the sense of English "2 or so")

@zbraniecki
Copy link
Collaborator

I'd like to challenge that a bit :)

I'm afraid we might need to re-implement strict ES5 compatibility in one way or another. I don't think we can expect existing projects to go through the whole ordeal of updating their build pipeline just to be able to use a newer version of fluent and friends.

That's not exactly how we should phrase this conversation imho. Fluent is a modern JS project. It requires "es6" (or whatever name you want to use - ES2018?).
If your project relies on a tool that explicitly states it will not support ES6 - like UglifyJS2 - then probably it just is not a good match.

We can try to keep supporting ES5, but IMHO we should accept that we are an ES6 project. It's not unreasonable and we are not the only ones. Increasing number of projects expect modern JS environments and this trend will just continue. Being a small pre-1.0 project means that we either embrace it, or spend resources trying to maintain the backward compatibility with increasing number of issues (Will we reject a build system tool that works for us, but is not compatible with es5? Will we reject an improvement to the system which does work with es5?).

I suggest we aim for two builds:

  • modern (nothing)
  • compat (transpiled according to the matrix)

@Pike
Copy link
Contributor

Pike commented Aug 9, 2018

I'd like to challenge that a bit :)

We have existing projects at mozilla that use Fluent to localize their work.

We can't just throw them under the bus.

@stasm
Copy link
Contributor Author

stasm commented Aug 9, 2018

I'm with Axel on this one. We benefit a lot from the fact that Fluent is already used in projects like Test Pilot and Common Voice. We get real-life validation which would be hard to get otherwise. I understand your position, @zbraniecki, and I think fluent.js is pretty good at embracing the future of ECMAScript. As you know, I worked hard to be able to drop support for old browsers. I'm very sad today to be forced to acknowledge that even when all major browsers support new ES, it's the build tools that don't. They were supposed to help use bridge this gap!

The vanilla & matrix approach is what we do right now, but it's not working for projects like Test Pilot. I agree that we should explicitly document that the ES5-transpiled builds are only intended for existing projects which already use Fluent. Any new projects should at least the matrix-compatible builds, or in case of Gecko, the vanilla ones.

@zbraniecki
Copy link
Collaborator

We have existing projects at mozilla that use Fluent to localize their work.
We can't just throw them under the bus.

That's cool. They signed up for a ride with a work-in-progress pre-1.0 project in a dynamic environment that gets modernized. The division line between "We haven't updated project X to modern toolchain" and "We don't really maintain this project anymore" is very thin. I'd like to make sure whatever we decide here, we don't add work because of projects that are on es5 because they're staled.

In my experience most projects will continually upgrade their toolchains if they're active, and we should focus on those.

The vanilla & matrix approach is what we do right now, but it's not working for projects like Test Pilot

Did we talk to Test Pilot about it? Are we the only thing that pushes them to embrace es6 in their build system? Are there other reasons? Are they planning to, or will they stay on es5 forever?

@stasm
Copy link
Contributor Author

stasm commented Aug 9, 2018

Did we talk to Test Pilot about it? Are we the only thing that pushes them to embrace es6 in their build system? Are there other reasons? Are they planning to, or will they stay on es5 forever?

"We" filed mozilla/testpilot#3789 to start this conversation. I honestly think it's less work, objectively, for us to introduce an ES5-compatible build target than for any of the projects using Fluent to upgrade their entire toolchain.

@zbraniecki
Copy link
Collaborator

I honestly think it's less work, objectively, for us to introduce an ES5-compatible build target than for any of the projects using Fluent to upgrade their entire toolchain.

If the equation is just that. But it may be a false dichotomy which is what I asked about. If Test Pilot is under pressure to update their toolchain from multiple sources, then maybe we're just part of the wave. If we're the only ones that would pressure them, and they're an active project interested in updating Fluent but at the same time committed to sticking to es5 level toolchain, then I agree.

@stasm
Copy link
Contributor Author

stasm commented Aug 10, 2018

Would it help to run a couple of integration tests on each bundle in a couple of node versions?

@Pike It all boils down to which polyfills are available. We currently require the polyfills before tests even run, which is a very effective way of hiding any potential failures. OTOH, the transpiled builds are not supposed to work without polyfills. Do you have a suggestion for how the integration tests you mention would look like?

@Pike
Copy link
Contributor

Pike commented Aug 10, 2018

AFAICT, the tests all import from ../src. Some highlevel tests that actually import compat, fluent, and vanilla, run after build in all could catch build errors. Not sure if the trouble we had was related to our build, though.

@stasm
Copy link
Contributor Author

stasm commented Aug 10, 2018

I meant this: https://github.com/projectfluent/fluent.js/blob/master/mocha_setup.js. Or this: https://github.com/projectfluent/fluent.js/blob/master/fluent/test/index.js#L4.

I'm not sure testing infra can solve this. It won't save us from another bug in UglifyJS. As for different node versions, perhaps we should focus on documenting which versions of node require which polyfills.

@stasm
Copy link
Contributor Author

stasm commented Aug 16, 2018

I'd like us to improve the effectiveness of our tests, but I don't want to block this issue on it. I'm going to start on the implementation tomorrow.

@stasm
Copy link
Contributor Author

stasm commented Aug 17, 2018

Note to self: the ES5-compatible builds will suffer from the regeneratorRuntime problem again. To recap: async functions are transpiled to generators which are transpiled to state machines which rely on regeneratorRuntime from babel-polyfill being available as a global. It needs to be available both on the clientside and during the build process, which sometimes is harder to achieve than it should be.

Most consumers rarely need both sync and async versions of mapContext* functions. The same applies to CachedSyncIterable and CachedAsyncIterable. I suggest we provide two separate build targets for the compat builds of fluent-sequence and cached-iterable: compat/sync and compat/async, each bundling only their respective functions. All regeneratorRuntime-dependent code will be contained to compat/async, making compat/sync safe to import from.

@stasm
Copy link
Contributor Author

stasm commented Aug 17, 2018

the ES5-compatible builds will suffer from the regeneratorRuntime problem again.

Actually, maybe not. I'm experimenting with ES5-compatible builds and as long as transform-regenerator is explicitly excluded, the compat builds keep using native generators.

I've now narrowed down the issue to uglify-es (which supports the function* syntax) vs. uglify-js (which doesn't).

@stasm
Copy link
Contributor Author

stasm commented Aug 17, 2018

I feel like I'm slowly piecing together the full picture. Fluent's compat builds use some ES6+ features, which old versions of Uglify don't support. From what I was able to gather, there are three Uglify packages on npm:

  • uglify-js is the master branch and it only supports ES5.
  • uglify-es is built from the harmony branch of the same repo as uglify-js and it supports some ES6+. Fluent compat files minify fine with it. Unfortunately, uglify-es is not maintained any more.
  • terser is a fork of uglify-es and is actively maintained. Fluent's compat builds minify fine using terser.

Uglify packages are rarely used by themselves. Instead they are often part of some other build tool, like Webpack or Gulp. And while uglifyjs-webpack-plugin depends on uglify-es, gulp-uglify still uses the old uglify-js package.

I found gulp-uglify-es which depends on terser. It should be possible to use it as a replacement for the old gulp-uglify. If this is true, maybe we don't need to change the compat builds at all.

@stasm
Copy link
Contributor Author

stasm commented Aug 20, 2018

I'm going to put this issue temporarily on hold. We can't reliably test upgrading to fluent 0.6+ without first clearing the way for the fluent-react upgrade. I'll first fix the issue preventing fluent-react upgrades and I'll then come back here if there's still a need to change how the compat builds are produced.

Details

Because of the MessageContext API changes in fluent 0.6 (formatToParts was removed), the two packages work best in the following combinations:

  • fluent == 0.4.x + fluent-react == 0.4.x, or
  • fluent >= 0.6.0 + fluent-react >= 0.6.0.

Unfortunately upgrading to fluent-react 0.6+ has been a challenge for projects which use server-side rendering because of #175. This issues has been recently fixed on master. I'd like to first publish a new version of fluent-react and a new version of fluent with all the recent renames (#222) and code moves (#212). As long as consumers don't use the old uglify-js package, there's a chance that the upgrade will not require ES5-compatible Fluent builds.

@stasm
Copy link
Contributor Author

stasm commented Sep 26, 2018

It turned out that changing the compat strategy wasn't needed after all. Fixing the server-side rendering blocker in #175 opened up the path to upgrading fluent and fluent-react in Test Pilot and Screenshots. This coincided with Test Pilot upgrading their build pipeline to Webpack 4 which also solved the minification problem. Common Voice and Send were able to upgrade, too. I kept track of all work involved in the Compatibility project.

As of today, all Mozilla web project using fluent or fluent-react are on the 0.8.x versions of both modules. The compat builds feature modern ES including classes, async functions and synchronous generator functions. We're no longer limited by the transpilation to ES5 which for features like sync and async iterators as well as sync functions relied on babel-polyfill and in particular, on the regeneratorRuntime.

Future upgrades of Fluent packages used in web projects will be much easier now because their build systems are now compatible with modern ES and we've verified this with the upgrade to fluent 0.8 and fluent-react 0.8.

@stasm stasm closed this as completed Sep 26, 2018
@stasm stasm mentioned this issue Dec 2, 2019
7 tasks
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