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

A draft conditional bundling approach #9661

Draft
wants to merge 14 commits into
base: v2
Choose a base branch
from

Conversation

marcins
Copy link
Contributor

@marcins marcins commented Apr 22, 2024

↪️ Pull Request

This is a partially working draft implementation of an approach for a specific type of conditional bundling that is designed for feature flags more than any other more general purpose "conditions".

The intention is that the end-user will not pay the download cost for code they will not execute because they do not have the feature flag enabled.

What it does

As a developer, you write:

const { Component } = importCond('my.feature.flag', './NewComponent', './OldComponent');

At runtime this will give you either the Component export from NewComponent or OldComponent. It is important to note that this is a synchronous import, it does not require a "tick" to return the module.

How it works

Currently we're essentially using the same underlying mechanisms as for dynamic imports to have Parcel produce new bundle groups for each of the "true" and "false" arms of the condition.

The importCond is replaced with a parcelRequire to a runtime asset that looks like:

module.exports = globalThis.__conditions['my.feature.flag'] ? parcelRequire('public_id_of_true_dependency') : parcelRequire('public_id_of_false_dependency');

It is up to the server to have:

  • evaluated the my.feature.flag feature flag and rendered the <script /> tag(s) for the correct bundle group (I mean, realistically it could render both bundle groups but that would negate the whole point of avoiding shipping the code)
  • rendered a <script /> that sets window.__conditions = { 'my.feature.flag' : (true|false) } for the runtime to use.
  • if there were to be a lot of conditions (like say hundreds) then the server would likely want to merge conditional bundles before serving them.. not sure if that's possible with the current output.

There is a Reporter that is currently in my test repo, that calls unstable_getConditionalBundleMapping in order to get the information needed for a server manifest.

What's missing from the draft implementation

  • Currently no support implemented for conditions inside async dependencies (this will require the runtime to load conditional bundles along-side async bundles)
  • Currently some hacks in ScopeHoistingPackager to ensure the dependency rewrites work correctly
    • Also to ensure the runtime code is in the right bundles
  • Currently doesn't support the same condition being used in multiple places, also haven't tested the same feature flag controlling multiple conditions (that should "just work?")

What I'm looking for

  • Feedback on the implementation, and where it could be improved.. there's a lot of different ways to do this (and we tried a few different approaches before settling on this one)
    • There's likely some better modelling / abstractions that could be used - currently it leans a lot on just treating conditional imports like dynamic imports for the purposes of bundling / packaging.

As this is all behind a feature flag, I'd still be keen on getting it merged at some point in order to allow us to experiment with it internally. I'll address some of the FIXMEs first though (mostly the hacks in the packager).

In the spirit of trunk based development, it would also be good to merge this even if it's not complete (see the "What's missing") rather than having a long running branch, so a focus on any blockers to that would be good (currently with the feature flag disabled there's mostly no impact except to some types, so it's a pretty safe change)

💻 Examples

🚨 Test instructions

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@devongovett
Copy link
Member

Do we need a new dependency priority here? It seems very similar to the parallel priority we already have – the dependency should load with the parent bundle group rather than lazily. This would also already work with async bundle groups. If a transformer were to transform importCond into something like this:

const { Component } = globalThis.__conditions['my.feature.flag'] ? require('./NewComponent') : require('./OldComponent');

where those require dependencies had priority: 'parallel' applied and perhaps a meta property declaring which condition they are used in. Then both bundles would end up in the same bundle group as the parent, and the runtime could decide which one to load. Maybe I'm missing something, but did you try that?

@marcins
Copy link
Contributor Author

marcins commented Apr 23, 2024

@devongovett I did have a look at the existing priorities but there was some reason that I thought parallel might not be suitable that I can't remember now.

If the conditional bundles were parallel, how would this affect any shared bundles produced? Without having a separate bundle group for each condition, doesn't this mean you're potentially loading unused shared code that might only be required by the condition untaken?

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

2 participants