Fix lazy compilation #9093

marcins · 2023-06-14T07:36:26Z

↪️ Pull Request

This fixes the reproduction provided in #8816 (Thanks @rayinaway), as well as a separate reproduction I created at https://github.com/marcins/lazy-testbed.

There were a couple of issues I found when trying to create a basic lazy compilation example:

The decision as to whether a bundle is a placeholder isn't revisited after the bundle is created. A change has been made to set the placeholder value to false when a bundle is requested.
Whether a bundle is a placeholder has been added to it's hash so that the bundle graph hash is invalidated when a bundle is no longer a placeholder.
When a previously deferred dependency was no longer deferred, the asset graph was updated correctly, but this didn't re-trigger bundling with incremental bundling. A change has been made to skip incremental bundling when a dependency is "un-deferred".

A new integration test based on my testbed reproduction has been added that I first verified was failing on current v2 branch, and verified that it now passes with these changes. In addition I tested the reproduction from #8816 and verified that Parcel with these fixes now loads those bundles correctly.

Note that due to my lack of deep knowledge in the areas touched the testing is possibly not exhaustive, I possibly haven't tested some scenarios that may still fail with these changes. I did manually test adding / removing dependencies and confirming that things re-compiled correctly.

Given the scope of the changes, they shouldn't affect anything outside of --lazy mode, so at least they're unlikely to break other functionality.

🚨 Test instructions

See repro repos above - these fail with current v2, but succeed when using Parcel linked with these changes.

✔️ 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

marcins · 2023-06-14T07:37:37Z

packages/core/core/src/AssetGraph.js

@@ -353,10 +353,12 @@ export default class AssetGraph extends ContentGraph<AssetGraphNode> {
        nodeId !== traversedNodeId
      ) {
        if (!ctx?.hasDeferred) {
+          this.safeToIncrementallyBundle = false;


Do any of the other cases here need this added?

The symptoms I saw was that while the asset graph was correctly updated after a previously lazy bundle was requested, the bundle graph was not - so I've focused on cases where dependency nodes have changed from deferred to not deferred, rather than asset nodes.

marcins · 2023-06-14T07:38:08Z

packages/core/core/src/Parcel.js

@@ -321,6 +321,10 @@ export default class Parcel {
            };
          }

+          // If this bundle was previously a placeholder, it now no longer is as it has
+          // been requested.
+          bundleNode.value.isPlaceholder = false;


Not sure if this is the right place or way to do this - is it "safe" to mutate a bundleNode here?

hmm kinda weird but probably ok since we only store the bundle graph in the cache when parcel exits.

Actually, I just had another look and I have a feeling this might no longer be required with the this.safeToIncrementallyBundle = false; fixes, as they'll cause the bundle graph to be recreated and the placeholder flag will be appropriately set.

My tests seem to pass without this line, I'll just validate it's the safeToIncrementallyBundle change that's fixing the issue.

Cool, I've removed this change as the other change (which I made later) in AssetGraph fixes the issue this was fixing.

marcins · 2023-07-24T21:33:07Z

packages/core/integration-tests/test/lazy-compile.js

+    assert(await distDirIncludes(['index.js', /^lazy-1\./, /^lazy-2\./]));
+  });
+
+  it('should lazy compile properly when same module is used sync/async', async () => {


This test still fails on this branch, it has been pulled in as an intentional broken test (hence the draft PR).

The "fix" is to remove the conditions for shouldBuildLazily in JSRuntime (e.g.:

parcel/packages/runtimes/js/src/JSRuntime.js

Lines 360 to 369 in 7ff54c7

// Also do this when building lazily or the runtime itself could get deduplicated and only

// exist in the parent. This causes errors if an old version of the parent without the runtime

// is already loaded.

if (

bundle.env.outputFormat === 'commonjs' ||

bundle.env.isLibrary ||

options.shouldBuildLazily

) {

externalBundles = [mainBundle];

} else {

)

With the code as it is, the whole bundle group doesn't get loaded - so when the uses-static-component-async bundle loads, it doesn't trigger a load of the static-component bundle, so it can't resolve the module.

I know that the"fix" (seen here: marcins@a36a37d) means that there is a potential issue when a bundle group grows after lazy compilation - this means Parcel can get into a state where it requires a browser refresh in order to recover. However, with the current state, a refresh won't heal as the load order is just incorrect.

As discussed in the Core meeting, the "fix" for this has now been cherry-picked onto this branch. I've updated the comments to leave behind some trace that there was lazy build specific code previously there.

jondlm · 2023-08-01T00:14:23Z

Hey @marcins I tried out this patch but I'm still seeing some more issues with lazy mode. Here's another minimal repro that's exhibiting similar issues to what this PR was meant to fix. Maybe it'll be helpful to shake out more bugs! I can also file this as a separate issue if that's helpful.

I'm still trying to get comfortable with parcel internals to propose fixes but I'm not quite there yet.

Thank you for all your hard work on --lazy, I'm also quite interested in seeing it work properly on a large codebase we're (Stripe) piloting Parcel out on.

devongovett · 2023-08-01T00:28:18Z

@jondlm did you build Parcel locally? This hasn't even been released in a nightly yet.

marcins · 2023-08-01T01:06:01Z

Hey @marcins I tried out this patch but I'm still seeing some more issues with lazy mode. Here's another minimal repro that's exhibiting similar issues to what this PR was meant to fix. Maybe it'll be helpful to shake out more bugs! I can also file this as a separate issue if that's helpful.

Thanks for that repro @jondlm - I haven't tried it, but looking at the description in the README that a refresh fixes it, I have a feeling you might've run into the issue which the conditions I removed in JSRuntime were trying to address.. with those conditions present, lazy compilation wouldn't work at all for our large application (even after refreshing), but without them it does work however I did intermittently see a "module not found" error that a refresh would fix. Presumably that's the issue the logic there was trying to avoid.

As I couldn't find a reliable way to minimally repro this, we decided to merge the fixes for now as they definitely make things better than before, albeit requiring an occasional refresh to unstick things. Having a repro that fails with these fixes in place will definitely be handy in understanding the issue and working on a proper fix.

But just to confirm as Devon asked - you still see this issue by running a locally built Parcel with this patch applied? If that's the case, then yes please - do file a separate issue.

jondlm · 2023-08-01T23:59:52Z

@devongovett I pulled commit 824ddbe, compiled the JS runtime, copied it into my project's node_modules, and then ran Parcel via bin.js. I think that should pull in all the relevant changes from this PR.

Thanks for the context @marcins. I'll file a separate issue with the case I found 👍

marcins · 2023-08-02T02:21:43Z

I pulled commit 824ddbe, compiled the JS runtime, copied it into my project's node_modules, and then ran Parcel via bin.js. I think that should pull in all the relevant changes from this PR.

You'll also need the changes in AssetGraph and BundleGraph to ensure bundles are rebuilt when they should be - though the symptoms of your issue (fixed by a refresh) definitely point to the JSRuntime part.

In any case, the latest nightly should now contain these fixes.

natan-stripe · 2023-08-03T22:58:54Z

👋 I created a fork of your original lazy-testbed repo with the steps to reproduce the additional issue that @jondlm and I are bumping into which he mentioned above. We've spent some time trying to figure out how to fix it but haven't really been able to understand the expected behavior and usage of deferred modules properly.

It seems like it's very close to working but there's just like one connecting piece that should be tying lazy async bundles to the relevant shared bundles that is maybe missing.

marcins commented Jun 14, 2023

View reviewed changes

marcins force-pushed the mszczepanski/fix-lazy-compilation branch from 2c22a39 to 92ee5eb Compare June 14, 2023 07:41

marcins marked this pull request as draft June 19, 2023 00:41

marcins force-pushed the mszczepanski/fix-lazy-compilation branch from 2eb85ac to 43d98d0 Compare July 24, 2023 06:34

marcins commented Jul 24, 2023

View reviewed changes

marcins marked this pull request as ready for review July 26, 2023 03:46

marcins requested a review from devongovett July 26, 2023 03:47

marcins self-assigned this Jul 26, 2023

devongovett approved these changes Jul 31, 2023

View reviewed changes

marcins added 6 commits July 31, 2023 16:35

Initial fixes for lazy compilation

2f3f498

Add failing integration test for lazy compilation

6327fb0

Add additional integration test for sync/async imports

28f5a90

Remove runtime conditions for lazy bundling

2f23cda

Update comments for JSRuntime changes

152aff7

Remove unnecessary change

a031a0f

marcins force-pushed the mszczepanski/fix-lazy-compilation branch from b58f23e to a031a0f Compare July 31, 2023 06:35

marcins merged commit 16deba5 into parcel-bundler:v2 Jul 31, 2023
16 checks passed

marcins deleted the mszczepanski/fix-lazy-compilation branch July 31, 2023 10:03

marcins mentioned this pull request Jul 31, 2023

parcel [serve] --lazy fails to load an async bundle under certain conditions #8816

Closed

natan-stripe mentioned this pull request Aug 3, 2023

Error with requiring modules because of incorrect sequencing for shared bundles when running in --lazy mode #9177

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lazy compilation #9093

Fix lazy compilation #9093

marcins commented Jun 14, 2023 •

edited

marcins Jun 14, 2023

marcins Jun 14, 2023

devongovett Jul 31, 2023

marcins Jul 31, 2023 •

edited

marcins Jul 31, 2023

marcins Jul 24, 2023 •

edited

marcins Jul 26, 2023

jondlm commented Aug 1, 2023

devongovett commented Aug 1, 2023

marcins commented Aug 1, 2023

jondlm commented Aug 1, 2023

marcins commented Aug 2, 2023

natan-stripe commented Aug 3, 2023

	// Also do this when building lazily or the runtime itself could get deduplicated and only
	// exist in the parent. This causes errors if an old version of the parent without the runtime
	// is already loaded.
	if (
	bundle.env.outputFormat === 'commonjs' \|\|
	bundle.env.isLibrary \|\|
	options.shouldBuildLazily
	) {
	externalBundles = [mainBundle];
	} else {

Fix lazy compilation #9093

Fix lazy compilation #9093

Conversation

marcins commented Jun 14, 2023 • edited

↪️ Pull Request

🚨 Test instructions

✔️ PR Todo

marcins Jun 14, 2023

Choose a reason for hiding this comment

marcins Jun 14, 2023

Choose a reason for hiding this comment

devongovett Jul 31, 2023

Choose a reason for hiding this comment

marcins Jul 31, 2023 • edited

Choose a reason for hiding this comment

marcins Jul 31, 2023

Choose a reason for hiding this comment

marcins Jul 24, 2023 • edited

Choose a reason for hiding this comment

marcins Jul 26, 2023

Choose a reason for hiding this comment

jondlm commented Aug 1, 2023

devongovett commented Aug 1, 2023

marcins commented Aug 1, 2023

jondlm commented Aug 1, 2023

marcins commented Aug 2, 2023

natan-stripe commented Aug 3, 2023

marcins commented Jun 14, 2023 •

edited

marcins Jul 31, 2023 •

edited

marcins Jul 24, 2023 •

edited