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

fix: run eslint from PATH #654

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Conversation

richardbarrell-calvium
Copy link
Contributor

Rationale:

  • npm and Yarn both put (a directory containing) the eslint binary on PATH during "npm run foo", so take advantage of that to invoke eslint in a way that is compatible with Yarn workspaces.
  • (Node that node_modules/eslint/... may not exist, because the module may have been installed in another node_modules further up the filesystem tree. Yarn does ensure that the binary gets symlinked into node_modules/.bin.)
  • This should fix issue gts does not work in a yarn workspaces monorepo #490.

@google-cla google-cla bot added the cla: yes label Jul 19, 2021
@richardbarrell-calvium
Copy link
Contributor Author

Hi! I'd like some feedback on this, please.

  • Does this work with everyone else's workflow? I believe it works fine with using gts by running the various scripts like npm run lint, npm run test and so on. I don't know for certain if there are other ways in which people invoke gts in the wild?
  • Were there any performance issues or anything like that which were being dealt with previously by invoking eslint from node_modules/eslint/bin/eslint?
  • I'm not aware of any reason why this in principle shouldn't work on Windows, but I don't have access to a Windows box to test this on. (I can get ahold of one in a few hours.)

@richardbarrell-calvium
Copy link
Contributor Author

richardbarrell-calvium commented Jul 22, 2021

@JustinBeckwith would you be a good person from whom to request code review on this, please? Tagging you because you mentioned PRs being welcome on the issue (#490) that I'm trying to solve.

@bcoe
Copy link
Contributor

bcoe commented Jun 10, 2022

@richardbarrell-calvium this change seems reasonable to me, but I would like to test in a few environments before landing.

Could I bother you to rebase and push, for whatever reason it seems like actions did not kick off.

@alexander-fenster
Copy link
Contributor

Alternatively, @richardbarrell-calvium, if you could check the box that grants contributor write access to your branch, we can drive this to completion. Thank you!

@alexander-fenster alexander-fenster self-assigned this Jul 21, 2022
Rationale:
- npm and Yarn both put (a directory containing) the eslint binary on PATH during "npm run foo", so take advantage of that to invoke eslint in a way that is compatible with Yarn workspaces.
- (Node that `node_modules/eslint/...` may not exist, because the module may have been installed in another `node_modules` further up the filesystem tree. Yarn does ensure that the binary gets symlinked into `node_modules/.bin`.)
- This should fix issue google#490.
@richardbarrell-calvium
Copy link
Contributor Author

Hi there! @bcoe apologies for not replying sooner, the email notification got buried in my inbox. My bad.

@alexander-fenster I've rebased my branch on main and pushed it. I'm afraid I have no idea where that checkbox is. I can grant you write access to https://github.com/calvium/gts from the "Collaborators and teams" page?

GitHub's UI is telling me that the CI actions weren't invoked because workflow approval is required. Here is a screenshot of what I'm seeing: Screenshot 2022-07-25 at 11 24 27

@aabmass
Copy link
Member

aabmass commented Jan 10, 2023

@alexander-fenster It would be great to see this move forward. If you're worried about compatibility, you could append ./node_modules/eslint/bin to PATH or manually fallback to the original invocation?

@naseemkullah
Copy link
Contributor

@danielbankhead could you please have a look at this one?

Copy link
Member

@danielbankhead danielbankhead left a comment

Choose a reason for hiding this comment

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

LGTM

@danielbankhead
Copy link
Member

@richardbarrell-calvium please pull from the base branch and I can get this merged.

@richardbarrell-calvium
Copy link
Contributor Author

@danielbankhead Thanks! I've merged main in to my branch and pushed.

@danielbankhead
Copy link
Member

Looking into the Windows test timeout...

@danielbankhead
Copy link
Member

@richardbarrell-calvium Nice! Looks like it passed. Do you mind pulling from main again?

@richardbarrell-calvium
Copy link
Contributor Author

I've merged main and pushed again.

@danielbankhead danielbankhead merged commit 5dc2a76 into google:main Sep 29, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants