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

Support selective version resolutions in esm-views #1872

Merged
merged 13 commits into from
Jun 28, 2022

Conversation

cristiano-belloni
Copy link
Contributor

@cristiano-belloni cristiano-belloni commented Jun 24, 2022

  • Manifest resolutions are formatted as a CSV in the [selectiveCDNResolutions] template field. Since external dependencies in esm.sh are a similar concept to selective dependencies resolutions in Yarn (they both lock a subdependency version through the whole dependency tree), it makes sense to have this courtesy template token.
  • Docs have been updated to explain how peerDependencyes resolution at CDN build time work and how the problem of multiple stateful dependencies is solved by the currently available CDNs.

@changeset-bot
Copy link

changeset-bot bot commented Jun 24, 2022

🦋 Changeset detected

Latest commit: 8035848

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
modular-scripts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coveralls
Copy link
Collaborator

coveralls commented Jun 24, 2022

Coverage Status

Coverage decreased (-0.1%) to 28.556% when pulling 8035848 on feature/esm-selective-version-resolution into 2c8586a on main.

@cristiano-belloni cristiano-belloni marked this pull request as ready for review June 28, 2022 11:24
particularly relevant in case the `peerDependency` in question is stateful:
suppose, for example, that one of your ESM Views depends on `react@17.0.1`, but
one of your dependencies on the CDN depend on `react@>16.8.0` (pretty common if
the dependency use hooks). Depending on the moment that yor dependency was first
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: your

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
the dependency use hooks). Depending on the moment that yor dependency was first
the dependency use hooks). Depending on the moment that your dependency was first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

CDN). [It also provides `[selectiveCDNResolutions]`](./esm-cdn.md), a template
string to automatically translate
[Yarn selective version resolutions](https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/)
to lists of locked dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it possible to have an example for each solution for the same React 16.8/17 example you referred to above? But this time showing the Modular solutions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

docs/esm-views/esm-cdn.md Outdated Show resolved Hide resolved
steveukx
steveukx previously approved these changes Jun 28, 2022
Copy link
Contributor

@steveukx steveukx left a comment

Choose a reason for hiding this comment

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

LGTM with a few typos

// Some CDNs support this mechanism - https://github.com/esm-dev/esm.sh#specify-external-dependencies
// This is especially useful if we have stateful dependencies (like React) that we need to query the same version through all our CDN depenencies
// We just output them as a comma-separated parameter in the CDN template as [selectiveCDNResolutions]
// TODO: possibly fallback to a filtered version of resolutions if this is not present
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this TODO in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks!

sgb-io
sgb-io previously approved these changes Jun 28, 2022
Copy link
Contributor

@sgb-io sgb-io left a comment

Choose a reason for hiding this comment

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

LGTM except for some small typos. I also added a suggestion to add an example

@cristiano-belloni cristiano-belloni dismissed stale reviews from sgb-io and steveukx via 12127ae June 28, 2022 13:07
cristiano-belloni and others added 4 commits June 28, 2022 14:07
Co-authored-by: Steve King <steve@mydev.co>
Co-authored-by: Sam Brown <sam@sgb.io>
Copy link
Contributor

@sgb-io sgb-io left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@cristiano-belloni cristiano-belloni merged commit e48bf7e into main Jun 28, 2022
@cristiano-belloni cristiano-belloni deleted the feature/esm-selective-version-resolution branch June 28, 2022 16:06
@github-actions github-actions bot mentioned this pull request Jun 28, 2022
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.

4 participants