You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
@bjohansebas was the goal to avoid using npm? this approach requires we add a passthrough "lint-staged": "lint-staged" script, which isnt something Im used to seeing.
I wanted to suggest using the --cwd arg to lint-staged and dropping the lint-staged script, but npm exec (npx replacement) wont be able to detect the installed bin if we don't help it discover a package.json
npm exec lint-staged --cwd astro
Im being pedantic here a bit, but we could instead drop the cd and the script by setting the npm prefix, which is how you tell npm to act as if it is in that dir and do its resolution from there:
npm exec --prefix astro lint-staged
but lint-staged does care about cwd for its config lookup, so to avoid cd we'd need...
The reason will be displayed to describe this comment to others. Learn more.
I accepted this because of performance. Since we are on Node 24, it is significantly faster than npx or npm run (it avoids the npm CLI startup overhead).
You're right that it requires the 'passthrough' script in package.json which is the trade off for that speed boost. I agree that the --prefix and --cwd alternatives feel a bit 'cursed' the current cd approach keeps it readable while staying fast. Once we move everything to the root, we can simplify this even further.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
Once the site is in the root, won’t this be necessary anymore? I think Husky already handles that for us. I just want to confirm that this is temporary and not something that will always stay there, because in all the Husky configurations I’ve had, this is the first time I’ve seen it done this way (or maybe my memory is failing me already 😅 the exhaustion haha).
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is temporary. Right now the project uses a subfolder structure, so Husky needs the path to the .git root. Once the site is moved to the root, we'll switch to the standard "prepare": "husky" command.
This will help to anyone contributing to redesign branch gets their local environment setup.
The reason will be displayed to describe this comment to others. Learn more.
@bjohansebas is your question whether the prepare script is temporary? Or the cd .. part of it?
prepare is necessary for husky to configure git to be aware of the git hooks. its something you can't keep in version control, the repo gitconfig to point to the script location (.husky). Their docs show this specific cd example for when the package.json is not at git root: https://typicode.github.io/husky/how-to.html#project-not-in-git-root-directory
It is one time setup, but setup that needs to be run whenever someone clones the repo fresh (like when a new contributor forks the repo).
It's one of the downsides to these kinda of git hook tools, there is setup that cannot be checked in and must be run on clone.
},
"dependencies": {
"@astrojs/mdx": "^4.3.13",
Expand All
@@ -39,12 +40,22 @@
"eslint-config-prettier": "^10.1.8",
"eslint-plugin-astro": "^1.5.0",
"eslint-plugin-jsx-a11y": "^6.10.2",
"husky": "^9.1.7",
"lint-staged": "^16.2.7",
"postcss-custom-media": "^12.0.0",
"postcss-nested": "^7.0.2",
"prettier": "^3.8.1",
"prettier-plugin-astro": "^0.14.1",
"typescript": "^5.9.3",
"typescript-eslint": "^8.53.1"
},
"lint-staged": {
"*.{js,jsx,ts,tsx,astro}": [
"eslint --fix",
"prettier --write"
],
"*.{json,md,css}": [
"prettier --write"
]
}
}
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Use
node --run.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.
Optimised in 2574c92
Uh oh!
There was an error while loading. Please reload this page.
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.
@bjohansebas was the goal to avoid using npm? this approach requires we add a passthrough
"lint-staged": "lint-staged"script, which isnt something Im used to seeing.I wanted to suggest using the --cwd arg to lint-staged and dropping the
lint-stagedscript, but npm exec (npx replacement) wont be able to detect the installed bin if we don't help it discover a package.jsonnpm exec lint-staged --cwd astroIm being pedantic here a bit, but we could instead drop the cd and the script by setting the npm prefix, which is how you tell npm to act as if it is in that dir and do its resolution from there:
npm exec --prefix astro lint-stagedbut lint-staged does care about cwd for its config lookup, so to avoid cd we'd need...
npm exec --prefix astro lint-staged -- --cwd astroThat sucks though and is cursed lol, so cd is fine for now.
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.
I accepted this because of performance. Since we are on Node 24, it is significantly faster than npx or npm run (it avoids the npm CLI startup overhead).
You're right that it requires the 'passthrough' script in package.json which is the trade off for that speed boost. I agree that the --prefix and --cwd alternatives feel a bit 'cursed' the current cd approach keeps it readable while staying fast. Once we move everything to the root, we can simplify this even further.