Skip to content

Mark previously deferred assets as dirty for symbol prop#9369

Merged
mattcompiles merged 5 commits into
v2from
fix-lazy-symbol-propagation
Nov 12, 2023
Merged

Mark previously deferred assets as dirty for symbol prop#9369
mattcompiles merged 5 commits into
v2from
fix-lazy-symbol-propagation

Conversation

@mattcompiles

@mattcompiles mattcompiles commented Nov 9, 2023

Copy link
Copy Markdown
Contributor

↪️ Pull Request

This PR fixes an issue in incremental symbol propagation + lazy mode builds where you have a lazy asset that depends on an asset which is also lazy itself. The issue occurs because propagateSymbolsDown is skipped but propagateSymbolsUp runs after the lazy bundle is requested. This can cause an incorrect "X does not export 'Y'" error to show as the usedSymbolsDown property has not been populated correctly.

flowchart LR
    Main -.-> Async
    Main -.-> SharedAsync
    Async --> SharedAsync
Loading

This fix here is to mark previously deferred assets and their dependencies as changed/dirty so symbol propagation knows to re-process them.

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

@mattcompiles mattcompiles marked this pull request as ready for review November 9, 2023 23:56
@marcins marcins self-requested a review November 12, 2023 22:20

@marcins marcins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! Love those problems that take days of digging only to result in a tiny change to fix.

}

async.js:
// Trigger more deps so a full 'propagateSymbolsUp' pass is executed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this only happens if the full pass runs? Feels odd to me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other circumstances that can cause this to occur outside of a full pass, however this was the easiest way to reflect in a test. The reason is fails though is because the full pass only applies to propagateSymbolsUp. However, in this case there are nodes that are not re-visited by propagateSymbolsDown and contain stale data.

@mattcompiles mattcompiles merged commit c784885 into v2 Nov 12, 2023
@mischnic mischnic deleted the fix-lazy-symbol-propagation branch November 13, 2023 10:44
@alshdavid alshdavid mentioned this pull request Nov 13, 2023
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.

3 participants