-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add "version" string and parsed "versionInfo" #1878
Conversation
Since @mjmahone @Neitsch @nodkz @martijnwalraven Can you please take a look? |
assert( | ||
packageJSON.version === require('../dist/version').version, | ||
'Version in package.json and version.js should match', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check guarantees that package.json
and version.js
are always in sync.
const { preReleaseTag } = parseSemver(packageJSON.version); | ||
if (preReleaseTag != null) { | ||
const [tag] = preReleaseTag.split('.'); | ||
assert(tag === 'rc', 'Only "rc" tag is supported.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the code had this check before, but I'm just curious why only rc
is supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martijnwalraven Thanks for the review 👍
It was added to prevent issues similar to #1375
These scripts are run by Travis and package automatically published so I'm trying to use "fail fast" design for them. Especially since I don't have NPM keys to do a manual fix if something goes wrong.
If we decide to use other tags in future it will be a matter of just adding them here.
This looks great! Really happy it pre-parses the version components so we don't have to do that at runtime. |
Looks great! Thanks a lot! |
Thanks for review 👍 |
Happened because graphql#1878 was authored before '14.3.1' release but merged after it. Also add 'npm run build' to CI to prevent simular problems in future
Happened because graphql#1878 was authored before '14.3.1' release but merged after it. Also add 'npm run build' to CI to prevent simular problems in future
Happened because #1878 was authored before '14.3.1' release but merged after it. Also add 'npm run build' to CI to prevent simular problems in future
Fixes #1726