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

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

Closed
stasm opened this issue May 30, 2018 · 7 comments
Closed

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

stasm opened this issue May 30, 2018 · 7 comments

Comments

@stasm
Copy link
Contributor

stasm commented May 30, 2018

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

  • CachedSyncIterable
  • CachedAsyncIterable
  • mapContextSync
  • mapContextAsync

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
Copy link
Contributor Author

stasm commented May 30, 2018

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
Copy link
Collaborator

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
Copy link
Contributor Author

stasm commented May 30, 2018

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
Copy link
Contributor Author

stasm commented May 31, 2018

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 d202fdd to the new repo. It's on master and not yet published as a new version.

@stasm
Copy link
Contributor Author

stasm commented May 31, 2018

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
Copy link
Contributor Author

stasm commented Jun 21, 2018

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 #222 before creating the new package in case we drop the name context.

@stasm
Copy link
Contributor Author

stasm commented Aug 17, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants