Simplify CLI version command to use version from package.json #2258
Conversation
META-DREAMER
force-pushed
the
simplify-version-command
branch
from
b611ab5
to
e8c586d
Compare
This will make it easier to keep the package version and the CLI version in sync by directly using the version in package.json instead of maintaining a separate versioning system. I also removed some of the extra formatting and git-state handling logic in the file since I didnt see it being used anywhere and @decentralion mentioned that it wasn't as necessary now that we have the NPM package as the main entrypoint for users. Test Plan: Run `sourcecred --version` and ensure it returns "v0.7.0-beta-16", or whatever is currently in the package.json
META-DREAMER
force-pushed
the
simplify-version-command
branch
from
e8c586d
to
26f9f38
Compare
| @@ -28,7 +28,7 @@ describe("cli/sourcecred", () => { | |||
| it("responds to '--version'", async () => { | |||
| expect(await run(sourcecred, ["--version"])).toEqual({ | |||
| exitCode: 0, | |||
| stdout: [expect.stringMatching(/^sourcecred v\d+\.\d+\.\d+$/)], | |||
| stdout: [expect.stringMatching(/^sourcecred v\d+\.\d+\.[a-z\d-]+$/)], | |||
There was a problem hiding this comment.
Added this to allow for versions such as 0.7.0-beta-16
There was a problem hiding this comment.
I think as written, it would be impossible to depend on the version in the frontend. However, showing version info in the frontend would be really nice. I've made a PR to do this: #2259. Can you try re-basing this on top of that PR, and see if it still works?
Yea, I was gonna implement that in a separate PR by injecting the version into the frontend build. This way might work too, but it would result in the package.json showing up in client side code, which is not ideal |
|
@decentralion Verified it works: |
There was a problem hiding this comment.
I’m okay with this. I do think that it can be really helpful for the
frontend to report the exact Git version at which it was built, and we
used to display that, but we don’t anymore. I’m happy to add it back in
later with an easier mechanism: e.g., setting it in an environment
variable at build time.
src/core/version.js
Outdated
| +dirty: boolean, // does the worktree have unstaged/uncommitted changes? | ||
| |}; | ||
| export type Environment = "development" | "production" | "test"; | ||
| const packageJSON = require("../../package.json"); |
There was a problem hiding this comment.
I’m curious—as you mentioned, this might pull package.json into the
frontend, which isn’t ideal (though not end of world); have you checked
whether it actually does, and if so whether destructuring here lets
Webpack tree-shake (const {version: packageVersion} = require(…))?
I’m not familiar enough with Webpack to guess whether this will help.
src/core/version.js
Outdated
| +dirty: boolean, // does the worktree have unstaged/uncommitted changes? | ||
| |}; | ||
| export type Environment = "development" | "production" | "test"; | ||
| const packageJSON = require("../../package.json"); |
There was a problem hiding this comment.
nit: packageJson, please; we use consistent camel-case except where
there’s a strong API reason not to (e.g., toJSON integration point).
Cf. JsonLog.
This commit prints the SourceCred version into the console on frontend load. This will be useful to help provide tech support for users, since we'll be able to find what version of SourceCred they'll be using. Test plan: Run `yarn start`, and check the console.
Following naming standards in codebase
This will make it easier to keep the package version and the CLI version in sync by directly using the version in package.json instead of maintaining a separate versioning system.
I also went pretty aggro in removing some of the extra formatting and git-state handling logic in the file since I didn't see it being used anywhere and @decentralion mentioned that it wasn't as necessary now that we have the NPM package as the main entrypoint for users. @wchargin if you think we should still keep that stuff around let me know!
Related: #2257
Test Plan: Run
sourcecred --versionand ensure it returns "v0.7.0-beta-16", or whatever is currentlyin the package.json