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

Refactor and clean up build configurations #5792

Merged
merged 9 commits into from
Sep 19, 2022

Conversation

limzykenneth
Copy link
Member

Resolves #5791

Changes:

Clean up and refactor code relating to the build and release process. Some changes will likely happen on the website repo as well. Main discussions on #5791.

PR Checklist

@limzykenneth
Copy link
Member Author

limzykenneth commented Sep 13, 2022

New release process that only uses github actions is also included now. Relevant documentation will be written later.

Will work on the following still, with some as informed by #5971:

  • Dependency updates
  • Remove prettier for build speed improvements
  • Replace static-fs with brfs
  • Remove babelify transform in favour of transpiling after bundling Remove grunt-babel in favour of babelify

@limzykenneth
Copy link
Member Author

limzykenneth commented Sep 14, 2022

For the latest updates, Uglify has been updated to the latest version and so it support ES6+ syntax now.

grunt-babel has been removed in favour of globally applying babelify this is because the source code must be transpiled before passing to browserify because browserify does not support ES6 modules (babel need to convert them first). Having just one transpilation step is more efficient.

static-fs babel plugin has been removed in favour of brfs-babel which does the same functionality of replacing fs.readFileSync calls to be inline content of the static files, just as a browserify transform instead of as a babel plugin (so globally applying babel won't have unintended effect). brfs-babel differs from just brfs by supporting ES6 import of fs so can be run before transpilation.

ESLint dependency on prettier has been removed with a set of new rules that matches existing behaviour subbed in, with minor modifications.


To remove prettier entirely is more difficult and ESLint doesn't seem to fix the compiled p5.js file as intended. More investigation into this is ongoing but I may call it at some point to go back to contributors documentation.

Replace prettier with pretty-fast in build generation.
Remove unreliable babel configuration.
@limzykenneth
Copy link
Member Author

Prettier has been successfully removed, although not with ESLint fix as that is as slow if not slower than prettier, but with pretty-fast which is the code formatter used by Firefox's console debugger. Formatting the final output of the code base is now at least 3 times faster.

The build overall should be a few seconds faster but will likely still take about 10 seconds to finish one version (with 3 versions in total, each taking different amount of time). To further optimize, we can consider replacing Babel with SWC, which I'm using in my own projects and is significantly faster, although with the plugins we are using it will be difficult to make that switch.

In any case, this PR is ready for review. @davepagurek @dhowe @endurance21 @jesi-rgb Do have a look if you are able to.

@limzykenneth limzykenneth marked this pull request as ready for review September 15, 2022 20:55
@jesi-rgb
Copy link
Member

Great, @limzykenneth! I'll review it as soon as I can :)

@endurance21
Copy link
Contributor

The explanation is awesome !

I would like to appreciate the amount of work and intelligence put here.

I have a basic doubt here, if we are transforming
"fs" to inline code before bundling, which essentially will not affect any files in node_midules, then what about libraries(present in node_modules ) which contains "fs" call syntax and they are being used by our codebase.

Does that also means that those "fs" calls will still be present after bundling ?

@limzykenneth
Copy link
Member Author

I have a basic doubt here, if we are transforming
"fs" to inline code before bundling, which essentially will not affect any files in node_midules, then what about libraries(present in node_modules ) which contains "fs" call syntax and they are being used by our codebase.
Does that also means that those "fs" calls will still be present after bundling ?

This is how the bundling currently works with static-fs as the babelify transform is not applied globally, only fs calls in our own codebase will be replaced with inline content. This is not a problem because 1. the dependencies are not expected to have fs calls that need to be inlined like this and, 2. even if the fs calls are present, they should be behind unreachable code for front end libraries. In other words, the dependencies we use should have accounted for this problem already.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

This looks good to me! I tried a local build and it all works as expected. Thanks for taking this on!

src/core/transform.js Outdated Show resolved Hide resolved
@limzykenneth limzykenneth merged commit eef4ce6 into processing:main Sep 19, 2022
@Qianqianye
Copy link
Contributor

Thank you all! 🚀

@joeyfurness
Copy link

(I'm not sure if this is the right place to bring this up, but this is the issue thread for the commit in question)

When trying to clone/fork/build p5.js on my Windows dev environment, I ran into this eslint error from the linebreak-style rule in .eslintrc.json: Expected linebreaks to be 'LF' but found 'CRLF'.

I noticed that in commit eef4ce6 that was merged from this issue, the .eslintrc file was updated to include this linebreak-style rule, which was not included in any of the other linting configs in previous commits. This conflicts with how the sample-linter.js was refactored in this same commit as well, where even if I set core.autocrlf=false in my git config to remove the CRLF line endings, I still get this error when running the npm run lint:sample command: eslint Cannot read properties of undefined (reading 'prefixLength'). (I'm pretty sure this is because it's expecting the line endings to be CRLF which it's getting from the import { EOL } from 'os')

I'm not sure if enforcing LF line endings was intended, but from all of the previous docs and issues I've seen, keeping core.autocrlf=true is the intended approach. Commenting out that rule also fixes the issue when that git config property is set.

@limzykenneth
Copy link
Member Author

@josephmwells Enforcing LF line endings is intended and we had issues before where git config core.autocrlf=false still allows some unintended CRLF to be added (for example where CRLF are encoded into string \r\n on build).

sample-linter.js should probably be refactored to not use system default EOL, the refactor I did was just moving const EOL = require('os').EOL; define further down the file before to be import { EOL } from 'os'; at the top, didn't really think about what it was doing back then.

@joeyfurness
Copy link

@limzykenneth Yah that makes sense, I tend to default to LF line endings across my dev environments since I am often switching between Windows and Unix. From what I've gathered crawling through old issues and commits, there was a very opinionated maintainer who felt that core.autocrlf=true should be maintained. I think before this commit, LF line endings were not actually enforced, but system line endings were (via sample-linter.js).

I'm currently just conducting some tests on the p5.js source right now, so it's not a huge blocker for me, but it would be nice if there was some remedy to this that specified the LF line endings opinion in code, and having that documented somewhere since a lot of Windows developers have core.autocrlf=true by default. Maybe adding a .gitattributes to specify that to false ?

@limzykenneth
Copy link
Member Author

@josephmwells Would you like to open a new issue to discuss this in more detail. We can include more contributors that way as a closed PR is not very visible.

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.

Library build and dependencies refactoring
6 participants