Skip to content

fix: isolated mode code cleanup#9055

Merged
wraithgar merged 1 commit intolatestfrom
gar/more-symbols
Mar 4, 2026
Merged

fix: isolated mode code cleanup#9055
wraithgar merged 1 commit intolatestfrom
gar/more-symbols

Conversation

@wraithgar
Copy link
Member

This started as a removal of the Symbols left in isolated reifier and ended as a bit of a cleanup/refactor.

Inline functions were moved to either static ones or class functions, forEach was changes to for loops where possible, comments were unwrapped, and more.

@wraithgar wraithgar requested a review from a team as a code owner March 3, 2026 18:29
@wraithgar wraithgar changed the title fix: code cleanup fix: isolated mode code cleanup Mar 3, 2026
@wraithgar
Copy link
Member Author

@manzoorwanijk Heads up we have decided to clean this file up now that we're looking more deeply at it lately. It may collide w/ other work you're doing but we'll try to handle merge conflicts as they come.

@manzoorwanijk
Copy link
Contributor

Thank you for the heads up.

@owlstronaut owlstronaut self-assigned this Mar 4, 2026
const hash = crypto.createHash('shake256', { outputLength: 16 })
.update(deps.join(','))
.digest('base64')
// Node v14 doesn't support base64url
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Node v14 doesn't support base64url

Can remove this now 😆


const proxiedIdealTree = this.idealGraph
async createIsolatedTree () {
await this.makeIdealGraph(this.options)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await this.makeIdealGraph(this.options)
await this.makeIdealGraph()

Looks like the argument was removed

Comment on lines 188 to 207
@@ -160,15 +206,19 @@ module.exports = cls => class IsolatedReifier extends cls {
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could these operations all be in the populateDeps guard so the work isn't done when populateDeps == false. Looks like the work is thrown away in that case

Copy link
Member Author

@wraithgar wraithgar Mar 4, 2026

Choose a reason for hiding this comment

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

Wow all of the edge walking belongs inside the guard

Copy link
Contributor

@owlstronaut owlstronaut left a comment

Choose a reason for hiding this comment

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

🥳

@wraithgar wraithgar merged commit 983742b into latest Mar 4, 2026
16 checks passed
@wraithgar wraithgar deleted the gar/more-symbols branch March 4, 2026 17:26
wraithgar pushed a commit that referenced this pull request Mar 4, 2026
This contains the changes from

- a2154cd (#8996)
- 880ecb7 (#9013)
- 26fa40e (#9041)
- 983742b (#9055)
- 10d5302 (#9051)
- a29aeee (#9028)
- 16fbe13 (#9030)
- 8614b2a (#9031)

Since Node 22 doesn't have npm 11 yet, it would be better to have this
change backported to v10


Also, we wish to use `install-strategy=linked` in the [Gutenberg
monorepo](WordPress/gutenberg#75814), which
powers the WordPress Block Editor. We are still on v10. So, these fixes
will help.
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