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

feat(gatsby): Normalize module keys between webpack plugin and loader #36644

Merged
merged 8 commits into from Sep 26, 2022

Conversation

tyhopp
Copy link
Contributor

@tyhopp tyhopp commented Sep 20, 2022

Description

Normalize module keys between the partial hydration webpack loader and plugin.

Previously the actual resource path was used, which caused problems because the different modules can be loaded in the loader and plugin based off of what is declared in package.json.

Documentation

N/A

Related Issues

[sc-55773]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 20, 2022
@tyhopp tyhopp added this to the Gatsby 5 milestone Sep 20, 2022
@tyhopp tyhopp added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Sep 20, 2022
@tyhopp tyhopp marked this pull request as draft September 20, 2022 09:30
@tyhopp tyhopp force-pushed the feat/partal-hydration-normalized-module-keys branch from 286566b to 4913bba Compare September 21, 2022 04:47
@tyhopp tyhopp marked this pull request as ready for review September 21, 2022 05:04
Base automatically changed from feat/partial-hydration-webpack to master September 21, 2022 11:39
@tyhopp tyhopp force-pushed the feat/partal-hydration-normalized-module-keys branch from a540d02 to cf45642 Compare September 23, 2022 03:44
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Oh, windows tests actually caught something:

Error: expect(received).toMatchSnapshot()

Snapshot name: `should normalize bare module imports from node_modules 1`

Snapshot: "file://node_modules/package-name"
Received: "file://Users/username/project/node_modules/package-name"

@tyhopp tyhopp force-pushed the feat/partal-hydration-normalized-module-keys branch from d8f01e6 to c1eb721 Compare September 26, 2022 02:16
@tyhopp
Copy link
Contributor Author

tyhopp commented Sep 26, 2022

Oh, windows tests actually caught something

Fixed in 8f11802

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM, had minor NIT but we can just go ahead and merge as-is

projectRoot
)

expect(normalizedModuleKey).toMatchSnapshot()
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I'd suggest using .toMatchInlineSnapshot() here given that output is not large, and that will collocate "expected" value to be here, making it easier to see what assertion is actually being done here (regular snapshot requires looking stuff up in .snap files etc).

Not big deal, just my preference for this kind of things

@pieh pieh dismissed LekoArts’s stale review September 26, 2022 07:58

changes have been made

@tyhopp tyhopp merged commit 81ca1dd into master Sep 26, 2022
@tyhopp tyhopp deleted the feat/partal-hydration-normalized-module-keys branch September 26, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants