-
Notifications
You must be signed in to change notification settings - Fork 7
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
Remove version #1410
Remove version #1410
Conversation
✅ Deploy Preview for fractal-framework-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
"build": "npm run graphql:build && NEXT_PUBLIC_GIT_HASH=`git rev-parse HEAD` next build", | ||
"build:windows": "for /F \"delims=\" %I in ('git rev-parse HEAD') do set \"NEXT_PUBLIC_GIT_HASH=%I\" && next 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.
Removed this, none of us are on Windows
"dev": "npm run set:env & next dev", | ||
"start": "npm run set:env & next start", | ||
"start:https": "npm run set:env & HTTPS=true next start", | ||
"start:windows": "set \"GENERATE_SOURCEMAP=false\" && for /F \"delims=\" %I in ('git rev-parse HEAD') do set \"NEXT_PUBLIC_GIT_HASH=%I\" && next start", |
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.
Removed this, none of us are on Windows.
Edit: this comment only applies to line 60.
"start": "npm run set:env & next start", | ||
"start:https": "npm run set:env & HTTPS=true next start", |
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 removed these, and these two scripts are what I'm most unsure about. When/who is ever running these start
scripts?
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 do technically use start
locally as I do like to test on the 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.
heard! I will replace these.
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.
start
also used by Netlify to run production server
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.
@mudrila are you sure about that? I see that netlify builds the site with npm run build
but I don't see anywhere in the settings where the start
script is executed...
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.
@mudrila as a test, I just removed the start
and start:https
scripts from package.json
again, and the deploy preview for this PR builds and deploys just fine.
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.
ok, adding the start
script back even tho it's just next start
@@ -53,13 +52,8 @@ | |||
"scripts": { | |||
"lint": "next lint", | |||
"lint:fix": "next lint --fix", | |||
"set:env": "GENERATE_SOURCEMAP=false NEXT_PUBLIC_GIT_HASH=`git rev-parse HEAD`", |
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.
Removed this script and inlined the important part (NEXT_PUBLIC_GIT_HASH=git rev-parse HEAD
) into the dev
script. Removed GENERATE_SOURCEMAP=false
because that was a CRA thing, not really relevant in our current project.
} | ||
textStyle="text-sm-mono-bold" | ||
> | ||
{process.env.NEXT_PUBLIC_GIT_HASH.substring(0, 7)} |
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.
The only real change here is that I'm showing a the git shorthash as the version identifier.
Closes #1409
This PR does a couple of things:
version
defined inpackage.json
to the git shorthash of the current build.scripts
inpackage.json
. Removed a few that (I don't think) anyone uses. Fixed some bugs in others.