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(core): Use lerna prepublishOnly version npm publish and use the … #11301

Merged
merged 5 commits into from
May 1, 2023

Conversation

stocaaro
Copy link
Member

@stocaaro stocaaro commented Apr 25, 2023

Description of changes

The reported version in metrics is currently based on the @aws-amplify/core version when it would be more idiomatic to report on the aws-amplify version and the version is trailing by one.

This change adds a prepublishOnly step to core is run by lerna between updating versioning and pushing the files to npm, then also updates the genversion command to source the version from aws-amplify instead of core.

This change also removes the version.ts file from core, adding it to .gitignore to make this a build artifact that isn't checked into git.

It is worth noting that this will now also differentiate preid releases, so a version like 5.1.4-unstable.0197e92.5 will be published going forward. I have confirmed with product that this is a positive outcome, not a problem to fix.

Why this change works

When running lerna publish, we want to rebuild version.ts in core after lerna versions all the packages, but before it publishes to NPM.

The lerna publish lifecycle scripts give us the opportunity to inject commands at different points in the lerna version and publish process. After versioning and immediately before npm release prepublishOnly is run if it exists on any releasable package. By adding prepublishOnly to @aws-amplify/core and having it run build for core, this ensures that the version.ts file is updated with the current aws-amplify package version.

Description of how you validated changes

I have tested the aws-amplify based versioning and publishing to verdaccio to test that the build artifacts match the expected values.

Checklist

  • PR description included
  • yarn test passes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@elorzafe
Copy link
Contributor

I have another idea, this should probably be generated but not versioned so lerna doesnt complain when this is generated and not commited. What do you think @stocaaro ?

@stocaaro stocaaro marked this pull request as ready for review April 28, 2023 15:10
@stocaaro stocaaro requested review from a team as code owners April 28, 2023 15:10
@stocaaro stocaaro marked this pull request as draft April 28, 2023 15:10
@stocaaro
Copy link
Member Author

Per Francisco's comment, the version.ts is now a build artifact, published to npm and maintained locally after yarn setup-dev that won't be checked in to git.

@stocaaro stocaaro marked this pull request as ready for review April 28, 2023 19:21
@stocaaro stocaaro requested a review from a team as a code owner April 28, 2023 19:21
Copy link
Contributor

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -23,11 +23,12 @@
"build:cjs:watch": "node ./build es5 --watch",
"build:esm:watch": "rimraf lib-esm && node ./build es6 --watch",
"build": "npm run clean && npm run generate-version && npm run build:esm && npm run build:cjs",
"generate-version": "genversion src/Platform/version.ts --es6 --semi",
"generate-version": "genversion src/Platform/version.ts --es6 --semi --source ../aws-amplify",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need that --source...?

Copy link
Member Author

Choose a reason for hiding this comment

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

This changes the version to be from aws-amplify instead of corresponding to @aws-amplify/core.

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Looks good!

Thanks @stocaaro 🎖️

@stocaaro stocaaro merged commit c682c5e into aws-amplify:main May 1, 2023
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