Skip to content

Conversation

@phated
Copy link
Contributor

@phated phated commented Jan 30, 2021

Issue: N/A

What I did

Sometimes packages needed by generators might not be published on the latest tag, so the wrong version would be added to the package.json when using a generator.

An example of this is vue-loader, which publishes v16 (with Vue 3 support) on the next tag. Upon adding vue-loader to a generator, it would add v15 to the package.json.

This adds support for generators to request versions in the string, like so:

baseGenerator(packageManager, npmOptions, options, 'vue3', {
    extraPackages: ['vue-loader@^16.0.0'],
});

How to test

  • Is this testable with Jest or Chromatic screenshots? The e2e tests will catch this once leveraged by a generator
  • Does this need a new example in the kitchen sink apps? ❌
  • Does this need an update to the documentation? ❌

If your answer is yes to any of these, please make sure to include it in your PR.

@phated phated added cli feature request run e2e extended test suite Run the e2e extended test suite in CircleCI workflows labels Jan 30, 2021
@shilman shilman added this to the 6.2 essentials milestone Jan 30, 2021
Copy link
Member

@shilman shilman 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 a corner case

)
packages.map(async (pkg) => {
const [packageName, packageVersion] = getPackageDetails(pkg);
return `${packageName}@${await this.getVersion(packageName, packageVersion)}`;
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT this will fail when packageVersion is a literal version. getVersion calls satisfies and if it fails it uses the latest version. Consider the case where latest is 1.3.5 and we need version 1.2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this only fails if the version doesn't exist. I tested semver.satisfies('1.0.0', '1.0.0') and it returns true so as long as the literal version exists inside versions.reverse(), it should find one.

That being said, the latestVersion method could probably be hardened against a constraint not being satisfied by all versions on npm. Do you want me to do that as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(current is only ever set for @storybook packages, so the versionToUse ternary only ever gets applied to those)

Copy link
Member

Choose a reason for hiding this comment

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

Aha my bad

@phated phated force-pushed the phated/versioned-extra-packages branch from cb4d642 to 354fc79 Compare January 30, 2021 01:00
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

💯

@shilman shilman merged commit 14a7f4b into storybookjs:next Jan 30, 2021
@phated phated deleted the phated/versioned-extra-packages branch January 30, 2021 01:51
@phated phated mentioned this pull request Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli feature request run e2e extended test suite Run the e2e extended test suite in CircleCI workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants