-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
docs: update module usage instructions to remove Grunt and reflect dev-2.0 tooling #7871 #7886
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
base: dev-2.0
Are you sure you want to change the base?
Conversation
Hi @LalitNarayanYadav thanks for working on this! To link the PR to an issue, please write: "Fixes #7871" (if the issue should be closed when this PR is merged; otherwise, write "Addresses [issue]"). Thank you! |
Thanks you @ksen0 ! I'll keep it in mind. |
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.
These looks good so far, Really thanks for your awesome work. I'll give another round of review. I just need to ask weather you are confirm with these Spanish docs? I'll review them with translating with google translator and AI since I don't know spanish?
npx eslint src/ | ||
npx eslint test/ |
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.
Yes, these commands look promising, and the npm run lint command appears to work correctly. Could you confirm whether changing the code’s indentation causes the linter to fail, or does it still pass? In my case, it always passes regardless of indentation.
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.
The linter isn't implemented in v2+ yet. See #7853
In v1+ it wasn't configured for .md files. So my best bet is to not indent the code snippets.
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.
Aah...okay. Thanks for letting me know @error-four-o-four :)
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.
Hi, I think we should also update the overview section. Currently it's something like:
An excellent new [feature](https://github.com/processing/p5.js/pull/2051) of p5.js allows user to build p5.js as a custom combination of its modules. This greatly helps in reducing the production version size of the library, as well as improves overall performance.
.
I think, the PR we link here adds a grunt task which takes in the arguments of modules it want to bundle together. Maybe we could remove/update it.
cd p5.js | ||
npm ci | ||
npm run grunt | ||
npm run grunt combineModules:module_x:module_y | ||
npm run build # builds the full p5.js bundle | ||
npm test # runs linter and tests using Vitest |
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.
This section is meant to show how to create a custom build of p5.js with selected components. Right now, the commands only build the full library and run the test suite. We should remove the final npm test line and replace it with the command that actually generates a custom bundle. I’m not sure whether the dev-2.0 branch already supports modular builds, @limzykenneth , could you confirm if dev-2.0 lets users assemble p5.js from a chosen set of modules, and if so, what the exact command is?
cd p5.js | ||
npm ci | ||
npm run grunt | ||
npm run grunt combineModules:module_x:module_y | ||
npm run build # builds the full p5.js bundle | ||
npm test # runs linter and tests using Vitest |
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.
"runs linter and tests using Vitest" also, this line is misleading. Vitest never “runs the linter”, if you look at the script section, we have two different commands,
"scripts" : {
"test": "vitest",
"lint": "eslint .",
},
so, for lint we should write npm run lint
, however we can probably remove the commands which is used for running tests in p5js from custom_p5_build.md
?
Changes:
npm run build
for dev-2.0 and guidance on custom builds via Rollup.Fixes #7871
PR Checklist
npm run lint
passes