projectfluent / fluent.js

JavaScript implementation of Project Fluent
https://projectfluent.org/
Apache License 2.0
934 stars 77 forks source link

Move Cached*Iterable and mapContext out of the fluent package #212

Closed stasm closed 6 years ago

stasm commented 6 years ago

The fluent package currently exposes the following exports (among others):

The async exports have been a source of upgrading pains for web projects targeting older browsers. The transpiled fluent/compat build requires the regeneratorRuntime provided by babel-polyfill which can significantly increase the bundle size of a web app. This was the reason why we created the fluent 0.4.x branch which uses a Syntax 0.5-compatible parser but doesn't add the async iterables to the API.

I'd like to fix this by moving the iterables and the mapContext methods out of the fluent package, so that fluent can be updated independently of these iteration helpers.

stasm commented 6 years ago

As the first step, I'd like to move Cached*Iterable to a new package called cached-iterable. It is agnostic to Fluent and may be useful for other codebases as well. I'll move the code into projectfluent/cached-iterable.

As far as mapContext* methods go, I think it should be possible to rewrite them without using the for await syntax. This would avoid requiring babel-polyfill with the regeneratorRuntime. mapContextAsync would be broken in older browsers, but at least it would be possible to import the package safely if only mapContextSync is needed. I'm still not sure if we should move both methods to a new package. I don't expect them to change much so perhaps it's better to keep them separate. Proposed names of the package: fluent-map, fluent-fallback, fluent-sequence.

@zbraniecki I'm looking for a second opinion here. Does this look like a good plan forward?

zbraniecki commented 6 years ago

I'm totally with you on moving CachedIterable to a new package. It's a micropackage, but not very different from https://www.npmjs.com/package/intl-format-cache

As to mapContext* I struggle a bit more. I'm not sure where they should live, and I'd like to minimize the damage the temporary state we're in imposes on our code base. In other words - assuming that by the end of this year, all environments we target will support async iterators, will we want to keep it in a separate package? If not, how can we make it easy to bring it back once the reason disappears?

stasm commented 6 years ago

If my attempt to rewrite mapContext* without for await succeeds, that the compatiblity problem will be solved. OTOH, by keeping those methods in fluent, we make fluent-react depend on the entire fluent package, whereas in fact it only needs mapContextSync. By moving the mapContext* methods to a separate package we could make fluent a peer dependency of fluent-react. This in turn would make it easier to update codebases using fluent-react. In bug 1450071 we also wouldn't need to vendor the entire fluent package.

stasm commented 6 years ago

I created https://github.com/projectfluent/cached-iterable and published version 0.1.0 which is the same as CachedIterable from fluent 0.6.4 (the last published version). I also ported https://github.com/projectfluent/fluent.js/pull/191/commits/d202fdde73efb57c4ba505a856d90a8c69d9e50e to the new repo. It's on master and not yet published as a new version.

stasm commented 6 years ago

Sadly, the new cached-iterable package also suffers from the regeneratorRuntime dependency problem. The problem is related to the browser support of async functions, generators and classes.

For browsers which don't support async functions, Babel uses a short function called _asyncToGenerator which wraps the original code transformed to a generator function (function*).

For older browsers which don't support generators, Babel falls back on to the regeneratorRuntime. However, this is still all happening inside of CachedAsyncIterable methods. As long as these methods are never called (because maybe you're only intersted in CachedSyncIterable, nothing breaks.

Next, for even older browsers, Babel also transpiles the class syntax into _createClass. Somehow, this moves the regeneratorRuntime into the top level code which runs when the methods of the class are defined. If babel-polyfill is not available, this throws a ReferenceError.

The entire problem boils down to the list of browsers that we support. That's #133.

stasm commented 6 years ago

Good news! #133 landed last week and with the updated browser support matrix, the regeneratorRuntime is gone from compat builds!

I think there's still value in moving mapContextSync and mapContextAsync to a separate package. It will make the dependency graph of bindings modules like fluent-react smaller. fluent-react only needs mapContextSync and in fact, apps using it can ship with other implementations of MessageContext. We're already seeing this in Firefox Devtools which use the MessageContext from intl/l10n/MessageContext.jsm.

I'll wait a few days for the feedback in https://github.com/projectfluent/fluent.js/issues/222 before creating the new package in case we drop the name context.

stasm commented 6 years ago

In #273 I moved mapContextSync and mapContextAsync out of the fluent package. They can now be imported from fluent-sequence.