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.

Sign up for GitHub

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

Jump to bottom

refactor(experimental): add lint:fix #1568

Merged
merged 1 commit into from
Sep 7, 2023
Merged

refactor(experimental): add lint:fix #1568

merged 1 commit into from
Sep 7, 2023

Conversation

2501babe
Copy link
Member

@2501babe 2501babe commented Sep 4, 2023

this creates a toplevel command to run the linters, feel free to chastize me if ive invoked to tools in a scurrilous manner

buffalojoec
buffalojoec approved these changes Sep 4, 2023
View reviewed changes
Copy link
Collaborator

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Lgtm!! Thanks

mcintyre94
mcintyre94 reviewed Sep 5, 2023
View reviewed changes
turbo.json Outdated
@@ -36,6 +36,10 @@
"inputs": ["rollup.config.types.js", "tsconfig.*", "src/**"],
"outputs": ["declarations/**", "dist/**/*.d.ts", "lib/**/*.d.ts"]
},
"lint:fix": {
"inputs": ["src/**", "test/**"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be passing test/* to the eslint --fix and prettier -w calls too?

Copy link
Member Author

Choose a reason for hiding this comment

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

there arent any test/ dirs tho, other than library-legacy (which i didnt add this command for because it has its own flows and is a separate library). everything is in src/

Copy link
Member Author

Choose a reason for hiding this comment

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

i removed test/** from this bc id just naively copy-pasted it

steveluscher
steveluscher approved these changes Sep 5, 2023
View reviewed changes
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise thanks!

packages/addresses/package.json Outdated
@@ -36,6 +36,7 @@
"compile:js": "tsup --config build-scripts/tsup.config.library.ts",
"compile:typedefs": "tsc -p ./tsconfig.declarations.json",
"dev": "jest -c node_modules/test-config/jest-dev.config.ts --rootDir . --watch",
"lint:fix": "pnpm eslint --fix src/* && pnpm prettier -w src/*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Running these serially kneecaps Turborepo from giving us MAX PARALLELISM. Make test:lint:fix and test:prettier:fix and then make a root task "style:fix": "turbo run test:lint:fix test:prettier:fix"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to suggest this too but what does turborepo do if you run two jobs in parallel that modify the same set of files? Is it smart enough to make that work without them ever colliding on the same file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was about to write something something race conditions something something file locks but now that I think about it for two seconds, Prettier has to run last in case Eslint has fixes.

Yeah OK ignore me parallelism doesn't make sense here.

Still prefer the term "style:fix" though because Prettier isn't really lint.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The ideal situation would be to get Prettier to run only on changed files, but at this point I think we've probably collectively burned enough time on this.

@2501babe
Copy link
Member Author

2501babe commented Sep 7, 2023

i believe this is now failing because ts doesnt like something that babel is doing...?

  cache miss, executing b909c70e542c957a
  
  > @solana/web3.js@0.0.0-development compile:js /home/runner/work/solana-web3.js/solana-web3.js/packages/library-legacy
  > cross-env NODE_ENV=production rollup -c
  
  [!] TypeError: Cannot redefine property: File
      at Function.defineProperty (<anonymous>)
      at Object.<anonymous> (/home/runner/work/solana-web3.js/solana-web3.js/node_modules/.pnpm/@babel+core@7.22.10/node_modules/@babel/core/lib/index.js:7:8)
      at Module._compile (node:internal/modules/cjs/loader:1241:14)
      at Object.Module._extensions..js (node:internal/modules/cjs/loader:1295:10)
      at Module.load (node:internal/modules/cjs/loader:1091:32)
      at Function.Module._load (node:internal/modules/cjs/loader:938:12)
      at Module.require (node:internal/modules/cjs/loader:1115:19)
      at require (node:internal/modules/helpers:130:18)
      at Object.<anonymous> (/home/runner/work/solana-web3.js/solana-web3.js/node_modules/.pnpm/@babel+core@7.22.10/node_modules/@babel/core/src/config/helpers/config-api.ts:4:1)
      at Module._compile (node:internal/modules/cjs/loader:1241:14)

its strange that this is happening now because i havent updated anything, i guess ci is fetching different dependency versions than a couple days ago?

@buffalojoec
Copy link
Collaborator

its strange that this is happening now because i havent updated anything, i guess ci is fetching different dependency versions than a couple days ago?

I just ran into this too on another PR. Is that what the problem is?

@2501babe
Copy link
Member Author

2501babe commented Sep 7, 2023 •
edited
Loading

yea its definitely either "babel updated doing something tsc doesnt like" or "tsc updated and decided it doesnt like babel anymore" because i get the exact same build failure in #1563 too now. either dependabot updated on red or the lockfiles arent locking everything...?

edit: builds fine for me locally of course

@2501babe
Copy link
Member Author

2501babe commented Sep 7, 2023 •
edited
Loading

ok its a bad dependabot commit, im pring a revert

@2501babe
Copy link
Member Author

2501babe commented Sep 7, 2023

apparently this is a bug in node? lol. i will leave it for you to do what you will angular/angular-cli#25782

@steveluscher
Copy link
Collaborator

apparently this is a bug in node? lol.

Yeah, just do what I do and ignore all of your problems until they go away.

steveluscher
steveluscher approved these changes Sep 7, 2023
View reviewed changes
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

giphy-6

@2501babe 2501babe merged commit 94b4d49 into solana-labs:master Sep 7, 2023
4 of 6 checks passed
@2501babe 2501babe deleted the 20230904_lint branch September 7, 2023 21:41
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.78.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mcintyre94 mcintyre94 mcintyre94 left review comments

@buffalojoec buffalojoec buffalojoec approved these changes

@steveluscher steveluscher steveluscher approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@2501babe @buffalojoec @steveluscher @mcintyre94