-
Notifications
You must be signed in to change notification settings - Fork 29
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
[WIP] Replaced deprecated neutrino config with updated webpack config #540
base: master
Are you sure you want to change the base?
[WIP] Replaced deprecated neutrino config with updated webpack config #540
Conversation
❌ Deploy Preview for firefox-performance-dashboard failed. Why did it fail? →
|
Great to see progress here. It looks like the deploy is failing with:
Is the idea to keep this PR open until tests/deploy are passing? If so, perhaps we should convert it to a draft until it's ready to land? I'll also remove myself from the reviewers as I don't feel qualified to review these changes. |
@davehunt Yes! It's a WIP so I'll be handling the rest of the errors. Once, it's in a good place, I'll remove the WIP flag. I wanted to put it up now to give visibility to work I'm doing on it. |
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.
Thanks for these updates!
I pointed out some suggestions and problems. The biggest problems is that the tests don't run :-) and when run they are failing.
But IMO it's fine to have tests failing for now, while you're working on fixing things.
Before landing, I think this should be done:
- simplify the webpack configuration by having just one file and use process.env.NODE_ENV, and get the configuration right
- get tests running (but failing)
- figure out which of .nvmrc or the node_version in netlify.toml is needed and keep one of them if both work (I don't mind which one, I'll let you decide)
The other suggestions can be done in follow-ups IMO.
@@ -2,3 +2,6 @@ | |||
from = "/*" | |||
to = "/" | |||
status = 200 | |||
[build] | |||
node_version = "18.18.0" |
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.
is this needed with the `.nvmrc? My understanding is that one or the other is needed but not both, and I'm afraid of seeing them different in the future :-)
"eslint-plugin-jest": "27.0.0", | ||
"eslint-plugin-jest-dom": "^5.5.0", | ||
"eslint-plugin-react": "^7.37.3", | ||
"eslint-plugin-testing-library": "^7.1.1", |
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.
note that you're not loading the eslint plugin for testing-libary atm (but that's OK, you can always enable it later)
The best way to reduce the number of critical dependency errors was to remove the deprecated neutrino config and replace it with a normal webpack config. Other changes included changing the jest and eslint config since they also used neutrino. Will continue to work on both in this PR to get the dashboards closer to something more modern and stable. For now, the app runs locally at the port 5000 and the netlify deploy is fixed.
Updates: