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

Task/update node, prettier, eslint #3786

Merged
merged 7 commits into from
Jul 16, 2024
Merged

Task/update node, prettier, eslint #3786

merged 7 commits into from
Jul 16, 2024

Conversation

jmetev1
Copy link
Collaborator

@jmetev1 jmetev1 commented Jul 16, 2024

this pr

  1. updates node to 20
  2. updates eslint
  3. updates prettier
  4. yarn.lock sources got changed to registry.npmjs. i guess that's a problem? or not?

all the changes in json data files are due to new prettier.

closes #3516
closes #2989

Test Links:

Landing Page
MoreCast
Percentile Calculator
C-Haines
FireBat
FireBat bookmark
Auto Spatial Advisory (ASA)
HFI Calculator

@jmetev1 jmetev1 requested review from conbrad and dgboss July 16, 2024 20:02
@@ -71,8 +71,8 @@
"cypress": "start-server-and-test start:cypress 3030 cy:open",
"cypress:ci": "start-server-and-test start:cypress 3030 cy:run",
"eject": "react-scripts eject",
"lint": "eslint './src/**/*.{ts,tsx}'",
"lint:fix": "eslint --fix './src/**/*.{ts,tsx}'",
"lint": "eslint",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming this change widens the file types that are linted? So typescript, tsx, javascript, etc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops. i made comments on last pr and didn't move them. one sec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved all to eslint.config to keep it in one place

@@ -1,4 +1,7 @@
module.exports = {
eslint: {
enable: false
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

by default creat react app runs eslint itself on each build. but since our ci is always doing that it seems unnecessary. this will speed up the build. plus it uses different eslint version so comes up with different problems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch, is it possible to add that explanation as a comment in the file?

@@ -1111,7 +1111,7 @@
"rate_of_spread": 0.001902355415765236,
"hfi": 1.8253517760238567,
"intensity_group": 1,
"sixty_minute_fire_size": 9.998822417249202e-07,
"sixty_minute_fire_size": 9.998822417249202e-7,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

another prettier thing. seems weird. is it ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are test fixtures, so as long as tests pass it's fine. Looks like just removing leading zeroes from floating point exponent notation.

"eslint-plugin-react-hooks": "^4.4.0",
"eslint": "^9.7.0",
"eslint-plugin-react-hooks": "^4.6.2",
"globals": "^15.8.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the eslint config file is added by a npx command. and then it uses some old version of globals that's floating around from other packages. which has a white space error that now gets linted so i manually installed this version. seems silly but...?

Copy link
Collaborator

@conbrad conbrad Jul 16, 2024

Choose a reason for hiding this comment

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

Yeah that's fine, thanks for catching and explaining this too.

Copy link

sonarcloud bot commented Jul 16, 2024

Copy link
Collaborator

@dgboss dgboss left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.39%. Comparing base (c9aae0e) to head (4e56648).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3786      +/-   ##
==========================================
+ Coverage   79.36%   79.39%   +0.02%     
==========================================
  Files         297      297              
  Lines       10882    10882              
  Branches      525      525              
==========================================
+ Hits         8637     8640       +3     
+ Misses       2100     2097       -3     
  Partials      145      145              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@conbrad conbrad merged commit 0d6a903 into main Jul 16, 2024
27 checks passed
@conbrad conbrad deleted the task/Update-Prettier-to-v3 branch July 16, 2024 21:32
vanislekahuna pushed a commit to vanislekahuna/wps that referenced this pull request Sep 19, 2024
1. updates node to 20
2. updates eslint
3. updates prettier
4. yarn.lock sources got changed to registry.npmjs. i guess that's a problem? or not?

all the changes in json data files are due to new prettier. 

closes bcgov#3516 
closes bcgov#2989
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.

Upgrade eslint to 9.0 Update Prettier to v3
3 participants