-
-
Notifications
You must be signed in to change notification settings - Fork 219
refactor: use release-please to update exported version
#714
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
base: main
Are you sure you want to change the base?
Conversation
| "scripts": { | ||
| "build": "rollup -c", | ||
| "build:update-version": "node tools/update-version.js", | ||
| "prepublishOnly": "npm run build:update-version && npm run build", |
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.
We're already running build before publishing:
js/.github/workflows/release-please.yml
Lines 44 to 47 in 04247cb
| - run: | | |
| npm install | |
| npm run build | |
| if: ${{ steps.release.outputs.releases_created == 'true' }} |
| import { fileURLToPath } from "node:url"; | ||
| import path from "node:path"; | ||
| import fs from "node:fs"; | ||
|
|
||
| // eslint-disable-next-line no-underscore-dangle -- Conventional | ||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| const { version } = JSON.parse(fs.readFileSync(`${__dirname}/../package.json`, "utf8")); |
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.
A note that this could be simplified:
| import { fileURLToPath } from "node:url"; | |
| import path from "node:path"; | |
| import fs from "node:fs"; | |
| // eslint-disable-next-line no-underscore-dangle -- Conventional | |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | |
| const { version } = JSON.parse(fs.readFileSync(`${__dirname}/../package.json`, "utf8")); | |
| import packageJson from "../package.json" with { type: "json" }; | |
| const { version } = packageJson; |
Or, similarly:
| import { fileURLToPath } from "node:url"; | |
| import path from "node:path"; | |
| import fs from "node:fs"; | |
| // eslint-disable-next-line no-underscore-dangle -- Conventional | |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | |
| const { version } = JSON.parse(fs.readFileSync(`${__dirname}/../package.json`, "utf8")); | |
| const { default: { version } } = await import("../package.json", { with: { type: "json" } }); |
Per https://nodejs.org/api/esm.html#import-attributes, import attributes have been supported since Node.js v20.10.0.
| import path from "node:path"; | ||
| import fs from "node:fs"; | ||
| import assert from "node:assert"; | ||
|
|
||
| // eslint-disable-next-line no-underscore-dangle -- Conventional | ||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| const { version } = JSON.parse(fs.readFileSync(`${__dirname}/../../package.json`, "utf8")); |
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.
Same note as #714 (comment).
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.
LGTM, thanks! I don't have any further comments.
(I didn't move it to second review needed since there are pending comments.)
Prerequisites checklist
What is the purpose of this pull request?
Simplifies the way
eslint-scopeandespreeare exportingversion.What changes did you make? (Give an overview)
Removed prepublish step that updates
version, and configured release-please to update it similar to how we're doing that in other projects (eslint/rewrite, eslint/json, eslint/markdown, eslint/css). Also added tests that should catch any problems with this when we manage to run CI on release-please PRs (#713).Related Issues
Is there anything you'd like reviewers to focus on?