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

Library build and dependencies refactoring #5791

Closed
limzykenneth opened this issue Sep 8, 2022 · 11 comments · Fixed by #5792
Closed

Library build and dependencies refactoring #5791

limzykenneth opened this issue Sep 8, 2022 · 11 comments · Fixed by #5792

Comments

@limzykenneth
Copy link
Member

limzykenneth commented Sep 8, 2022

Topic

As part of a refactoring of the build and release process, there are some options that may need some discussions on related to some discussions around the implementation of saveGif() @jesi-rgb has been working on.

  1. UglifyJS that we are using now is UglifyJS2 which does not support ES6 and later syntax. We'll need to replace it with either UglifyJS3 or terser. I'd personally prefer terser but would like to know if there's any other opinion about considering UglifyJS3 instead.
  2. Currently we are using babelify as a browserify transform for code transpilation while recently @jesi-rgb has added grunt-babel to do the same. The main question here is, ideally we will only do one transpilation, so should it be done before bundling everything (so only transpiling p5.js source code, ie babelify) or after (including node_modules dependencies, ie. grunt-babel)?

Just tagging a few people I can think of off the top of my head but if you have some thoughts please share.
@Qianqianye @stalgiag @davepagurek @endurance21

@jesi-rgb
Copy link
Member

jesi-rgb commented Sep 8, 2022

I believe the last tag @/endu is not correct, you probably meant @endurance21!

@jesi-rgb
Copy link
Member

jesi-rgb commented Sep 8, 2022

The main problem has been stated very clearly. Currently we are having trouble making the build and it all comes down (as far as I'm concerned) to uglify not liking new syntax.

Given the options, I think it's better to first bundle → transpile, which will cause less issues down the line since all the necessary code is already considered.

Looking forward to hearing from the community!

@limzykenneth
Copy link
Member Author

I believe the last tag @/endu is not correct, you probably meant @endurance21!

Thanks, corrected. Github autocomplete failed me 😄

@Qianqianye
Copy link
Contributor

Thanks @limzykenneth and @jesi-rgb. I am adding the Build Process stewards @outofambit and @kungfuchicken to this conversation.

@endurance21
Copy link
Contributor

I think the one of the main issue we encountered was with
"Static-fs", plugin that we are using.

This plugin is necessary to read the content of file and make it available inline while bundling.

If I am not wrong that plugin works with "babelify", so if we are intending to go with
Bundling ----then---transpiling , we would need to figure out the way static-fs will work.

Also some third party library is contains the "fs" calls too so we may need to perform static-fs work on those libraries also, but in current implementation the static-fs works only for our own codebase ( the default behaviour of browserify) and when we tried making it a global behaviour it thrown errors.

So just to wrap-up, we need to keep in mind that static-fs calls must be dealt when we are thinking any approaches to transpiling our code.

@limzykenneth
Copy link
Member Author

limzykenneth commented Sep 9, 2022

static-fs is basically replacing something called brfs when it is implemented, they both do the same thing which is replacing the fs calls to be inline content. brfs is also a browserify transform so it will be running before bundling and so won't affect node_modules code.

@dhowe
Copy link
Contributor

dhowe commented Sep 9, 2022

In addition to supporting current syntax, would be great to improve the build time (with, for example, $ npm run dev)
This (specifically the browserify:dev task) is currently so slow (close to 30 seconds on my system) as to make development rather painful

@limzykenneth
Copy link
Member Author

In addition to supporting current syntax, would be great to improve the build time (with, for example, $ npm run dev)
This (specifically the browserify:dev task) is currently so slow (close to 30 seconds on my system) as to make development rather painful

Ah yes, there's some findings on that as well. The two most notable things that takes the most time are

  1. The actual bundling (which currently includes babelify) of the library, ie. taking all the source and dependencies and put them into one file.
  2. Prettier formatting the bundled code after it has been generated.

It would be a good idea to look into removing prettier entirely as well since it's been a bit problematic with version updates, so this will be one priority for improving build times for me.

@limzykenneth
Copy link
Member Author

limzykenneth commented Sep 14, 2022

While trying to clean up the build some of the existing dependencies we have I find that, mainly polyfills, are supported by all our stated browsers except for Opera Mini. I had a quick look and found that Opera Mini supposedly doesn't run Javascript locally, which makes me doubt that it works with p5.js at all regardless of if we include the polyfills or not.

I can't test this as I don't have an Android device with me. Would someone here with an Android device try it out and confirm if p5.js (or even canvases) can work with Opera Mini at all?

@davepagurek
Copy link
Contributor

I just tried it out. It looks like in the default mode, it still runs local js for canvases. If you change the settings to "Extreme Savings Mode", I think in that case it runs no local js, and all canvases break.

@limzykenneth
Copy link
Member Author

I guess we'll leave it for now in that case. I'm going to have another go at trying to remove prettier and whether I manage to or not, #5792 should be ready for review and merge by then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants