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

Normalizes dependencies #4305

Merged
merged 9 commits into from Mar 31, 2022
Merged

Normalizes dependencies #4305

merged 9 commits into from Mar 31, 2022

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Mar 31, 2022

What's the problem this PR addresses?

Dependencies sometimes weren't normalized (with the npm: protocol), which made it difficult to know what input to expect in which place.

How did you fix it?

All dependencies must now be normalized prior to being returned by resolvers or hooks. The configuration class now provides helpers to this effect.

I changed the dependencies format from a Map<DescriptorHash, Descriptor> to a Record<string, Descriptor> so that resolvers can modify the descriptor without changing its key in the map, which would prevent followup consumers from accessing the dependencies (an alternative would have been to use an old DescriptorHash with a new Descriptor, but this seemed a confusing and bad idea).

A compatibility layer in the lockfile should make the transition fairly painless from an end user point of view.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz
Copy link
Member

merceyz commented Mar 31, 2022

I changed the dependencies format from a Map<DescriptorHash, Descriptor> to a Record<string, Descriptor> [...]

What's preventing you from making it a Map<string, Descriptor>?

@arcanis
Copy link
Member Author

arcanis commented Mar 31, 2022

What's preventing you from making it a Map<string, Descriptor>?

Nothing technically speaking, it's more a matter of semantics. Since with this diff the dependencies are "static" for a same resolver, I feel like it makes more sense to use an object and let resolver functions use e.g. destructuring, than having to go through the get method w/ static names (I know we already do it with Configuration#get, but that's something I'd like to change as well when I get time).

Using a map would be more backward compatible, but given that I'm changing a few other things in resolvers' interfaces anyway (like getSatisfying, in #4302), I'd tend to pick the cleaner approach rather than adding (admittedly small) legacy.

@arcanis arcanis merged commit 1fbd616 into master Mar 31, 2022
@arcanis arcanis deleted the mael/protocol-normalization branch March 31, 2022 15:29
@arcanis arcanis mentioned this pull request Mar 31, 2022
13 tasks
@skoging
Copy link

skoging commented Apr 28, 2022

I think this change has caused some weirdness with peer dependency validation:

❯ yarn explain peer-requirements pbf332
➤ YN0000: react-scripts@npm:5.0.1 [ffbb4] provides webpack@npm:5.72.0 [57b3f] with version 5.72.0, which doesn't satisfy the following requirements:

➤ YN0000: fork-ts-checker-webpack-plugin@npm:6.5.2 [62680] → >= 4     ✓
➤ YN0000: react-dev-utils@npm:12.0.1 [f1e1c]               → npm:>= 4 ✘

➤ YN0000: Note: these requirements start with react-dev-utils@npm:12.0.1 [f1e1c]

The npm:>= 4 is considered not met, while >= 4 is met.

First of all this is clearly incorrect, and also it does not seem to be fully normalized. Shouldn't both be npm:>= 4 now?

@arcanis
Copy link
Member Author

arcanis commented Apr 28, 2022

Peer dependencies aren't supposed to go through this normalization, because they only support Semver ranges (the define compatible versions, not the source from which those versions must be obtained) 🤔

Do you have a minimal repro I can copy/paste?

@skoging
Copy link

skoging commented Apr 28, 2022

Did some more testing and it appears to be caused by packageExtensions:

  react-dev-utils@*:
    peerDependencies:
      typescript: ">= 2.7"
      webpack: ">= 4"

So these extensions seem to be normalized, when they shouldn't be?

@skoging
Copy link

skoging commented Apr 28, 2022

yarn create react-app and adding these packageExtensions to .yarnrc.yml should reproduce this issue

@paul-soporan
Copy link
Member

@skoging The issue has been fixed in #4588.

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

4 participants