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

1197 npm build warnings in ci #1203

Draft
wants to merge 2 commits into
base: 2.3.0
Choose a base branch
from
Draft

Conversation

MyPyDavid
Copy link
Member

Description

Related issue: #1197

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

@MyPyDavid MyPyDavid self-assigned this Nov 27, 2024
Copy link
Member

@jochenklar jochenklar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice thanks for looking into it. However, I don't want the profile hints in watch mode. But I want the export 'default' warning. I thought about adding a warningsConfig in webpack.config.js, but not if args.watch is set. Or we add a build:testing with a testingConfig. What do you think?

webpack.config.js Outdated Show resolved Hide resolved
@MyPyDavid
Copy link
Member Author

yeah, also thought about adding another config. By adding a testingConfig I can keep the other Configs the same right?

@jochenklar
Copy link
Member

Yes, and testing is pretty common as environment. You just need an else if in the last block.

package.json Outdated
@@ -2,6 +2,7 @@
"name": "rdmo",
"scripts": {
"build:prod": "webpack --config webpack.config.js --mode production",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that build:prod fails on warnings and testing does not, but contains the performance hints.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive added an object ignorePerformanceWarnings that build:prod gets so that it can fail on other warnings

package.json Outdated
@@ -2,6 +2,7 @@
"name": "rdmo",
"scripts": {
"build:prod": "webpack --config webpack.config.js --mode production",
"build:testing": "webpack --config webpack.config.js --mode production --env testing --fail-on-warnings",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just --mode testing and add 'process.env.NODE_ENV': JSON.stringify('testing')?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I read the docs, I don't think it goes like that:
mode gets only string = 'production': 'none' | 'development' | 'production'
for --env I can pass anything

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, I did not check.

} else {
return merge(config, baseConfig, productionConfig)
throw new Error(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To much complexity production can be the fallback and should be very strict, but still needs to allow for larger files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, refactored it into a switch case, I don't see the need for a fallback when it should not be receiving any other mode..

Copy link
Member

@jochenklar jochenklar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should name it different from testing if the only difference is that the performance warnings are suppressed. build:warn maybe? It could also fail on warnings i guess. We will probaly not use it much. But you can also merge right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants