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: remove runtime dependency on load-rules #462

Merged

Conversation

llllvvuu
Copy link
Contributor

@llllvvuu llllvvuu commented Jul 30, 2023

dynamic loading/requiring of files breaks bundlers as those files will not be available at runtime of bundled code (#461)

i accomplish this by adding a script, npm run generate-rulesets, which generates a static version of the rulesets

i also add prepublish and precommit hook to make sure docs and rulesets are up-to-date if anything in the rules directory has changed

dynamic loading/requiring of files breaks bundlers as those files will
not be available at runtime of bundled code (protofire#461)

we accomplish this by adding a script, `npm run generate-rulesets`,
which generates a static version of the rulesets
@llllvvuu llllvvuu force-pushed the fix/static_require_rulesets branch from f0cddaf to 1538f1e Compare July 31, 2023 01:50
Copy link
Collaborator

@dbale-altoros dbale-altoros left a comment

Choose a reason for hiding this comment

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

why is this file solhint.js empty ?

"docs": "node scripts/generate-rule-docs.js"
"generate-rulesets": "node scripts/generate-rulesets.js && prettier --write conf/rulesets",
"docs": "node scripts/generate-rule-docs.js",
"prepare": "husky install",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry if it is a dumb question, but
are the 'prepare' and 'prepublishOnly' needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not strictly necessary, but it makes docs and ruleset JSON automatically updated when you commit or publish

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. I know... What I meant is:
Husky executes sh ./scripts/generate-changed.sh on every commit(pre)
That files is calling "generate-rulesets" and also "docs" npm commands (which is the whole purpose of it and it is a good thing)

Maybe I'm missing something but I don't see any call to 'prepare' and 'prepublishOnly'... That is why I asked

Copy link
Contributor Author

@llllvvuu llllvvuu Aug 1, 2023

Choose a reason for hiding this comment

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

Oh, I see what you're asking.

Those are npm lifecycle scripts. npm will run prepare when you run npm install, that makes sure that git is synced with husky. It's recommended here.

npm will run prepublishOnly when you run npm publish, that makes sure that npm package has up to date rulesets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ohh thanks for the clarification
I've never used those...
Thanks for the links as well...
I have one question if you dont mind..
I see in the link you provided the 'prepare'
Runs BEFORE the package is packed, i.e. during npm publish and npm pack
Runs on local npm install without any arguments
Runs AFTER prepublish, but BEFORE prepublishOnly

If I have in github actions to run E2E tests, the npm pack and also npm install and some more...
This means it will do a husky install on each step ?
Thanks!

@llllvvuu llllvvuu force-pushed the fix/static_require_rulesets branch from 1538f1e to 4b76b89 Compare August 1, 2023 12:33
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Aug 1, 2023

solhint.js was a mistake, reverted

@dbale-altoros dbale-altoros changed the base branch from master to develop August 3, 2023 16:24
@dbale-altoros dbale-altoros merged commit f4e96ee into protofire:develop Aug 3, 2023
10 checks passed
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.

None yet

2 participants