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

Document Makefile requiring dev NODE_ENV #10761

Closed
wants to merge 1 commit into from
Closed

Document Makefile requiring dev NODE_ENV #10761

wants to merge 1 commit into from

Conversation

ljoonal
Copy link

@ljoonal ljoonal commented Mar 18, 2020

Hopefully this'll be temporary, or even rejected over a better solution. To quickly summarize: Some npm devDependencies are required for the frontend build steps, and it doesn't seem to be documented that development node environment is required in order for make to work.

Longer explanation:

Due to some dependencies, like webpack-cli for example, being required to build the project whilst still being in the devDependencies of the package.json file, the project fails to build if someone has their default NODE_ENV set to production instead of development.

As of writing this, if NODE_ENV is set to production, eslint fails first, due to at least missing eslint-config-airbnb-base. Then webpack fails due to missing webpack-cli. Then stylelint fails. And lastly, postcss is missing.

Hopefully this'll be temporary, or even rejected over a better solution. To quickly summarise: Some npm devDependencies are required for the frontend build steps.

Longer explanation:

Due to some dependencies, like `webpack-cli` for example, being required to build the project whilst still being in the devDependencies of the `package.json` file, the project fails to build if someone has their default NODE_ENV set to production instead of development.

As of writing this, if NODE_ENV is set to production, eslint fails first, due to at least missing `eslint-config-airbnb-base`. Then webpack fails due to missing webpack-cli. Then stylelint fails. And lastly, postcss is missing.
@ljoonal ljoonal changed the title A fix for npm dependencies on prod NODE_ENV Document Makefile requiring dev NODE_ENV Mar 18, 2020
@jolheiser jolheiser added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/docs This PR mainly updates/creates documentation labels Mar 18, 2020
@silverwind
Copy link
Member

NODE_ENV=development will put webpack in development mode, which will skip minification and other steps. I think we have to move all build deps to dependencies to ensure npm will always install them.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 18, 2020
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 18, 2020
- move all JS build dependencies to 'dependencies'
- update all JS dependencies

Reason for this is that npm will only install 'dependencies' when under
the effect of NODE_ENV=production which may be present on some build
systems.

Linters currently need to be depdendencies because we run linting as
part of the build step, but I plan to move them to a separate 'lint'
target which means they can move to devDependencies then.

Fixes: go-gitea#10761
@silverwind
Copy link
Member

Fix for this in #10763.

@lunny lunny closed this in #10763 Mar 19, 2020
lunny pushed a commit that referenced this pull request Mar 19, 2020
- move all JS build dependencies to 'dependencies'
- update all JS dependencies

Reason for this is that npm will only install 'dependencies' when under
the effect of NODE_ENV=production which may be present on some build
systems.

Linters currently need to be depdendencies because we run linting as
part of the build step, but I plan to move them to a separate 'lint'
target which means they can move to devDependencies then.

Fixes: #10761
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants