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

use Parcel for build process #3431

Closed
wants to merge 12 commits into from
Closed

Conversation

outofambit
Copy link
Contributor

@outofambit outofambit commented Dec 30, 2018

for #3425. definitely seems more straightforward than configuring rollup (#3429)

Goals

  • reduce (manual) build process configuration by using parcel
  • remove uglify dependency
  • remove browserify dependency (combineModules task needs browserify for now)

Implementation Details

  • new parcel and parcel:min grunt tasks that replace browserify task
  • retained separate p5.dom minification task, but moved from ugify to terser(which parcel uses internally)
  • did not modify combineModules task, as it is more involved and would require significant changes to work with parcel. it will continue to use browserify for now
  • used environment variables (process.env) to set minification in bundle at build time
  • added grunt-banner dependency to add comment banners to outputted builds (parcel does not yet have good support for this, and it can be used across p5 and p5.dom building)

To Do

Left to do for this to be considered for merging:

@outofambit outofambit added the WIP label Dec 30, 2018
@outofambit
Copy link
Contributor Author

@limzykenneth what do you think?

@limzykenneth
Copy link
Member

This is definitely a lot better than the rollup concept, just one new package and the build file is so much cleaner. Also the tests are able to pass at least in the browser that's also extra points! Passing tests in command line is more complicated probably because of spawning a Chrome instance to run the test, it will be great to have that pass somehow.

I personally much prefer this to the rollup one just because of its straightforwardness and simplicity.

@outofambit outofambit changed the title Parcel build proof of concept use Parcel for build process Jan 8, 2019
@outofambit outofambit force-pushed the parcel branch 2 times, most recently from ab50600 to 4d423df Compare March 1, 2019 03:53
@outofambit outofambit removed the WIP label Mar 17, 2019
@outofambit
Copy link
Contributor Author

@limzykenneth i think this is ready now!! would you mind taking another look and let me know what you think?

@limzykenneth
Copy link
Member

Amazing! I'll take a look soon, will get back to you.

@limzykenneth
Copy link
Member

Looking great so far!

Couple things from me, first is that do we need to generate source maps? I'm thinking probably not as they need to be included by the users or it would throw 404 errors which can be confusing for beginners and inlining the source map will bloat out the already pretty big full version of p5.js.


If I understand correctly grunt-terser is just to minify p5.dom.js? Just asking to confirm, no problem for me if that's the case.


Another detail, does the build step needs to be separate out into two? (build and build-p5)

Is it a stylistic choice for clarity or does separating the steps have some functional purpose? Again, no major opinion about this, just curious.

@Spongman
Copy link
Contributor

i noticed that while es6 code in p5.js is transpiled into es5 (eg. const -> var), this is not the case for code in some of the 3rd-party libraries (search for const in p5.min.js in this branch).

the p5.min.js from master doesn't have this.

@outofambit
Copy link
Contributor Author

@limzykenneth thanks, these are all good points!

Couple things from me, first is that do we need to generate source maps? I'm thinking probably not as they need to be included by the users or it would throw 404 errors which can be confusing for beginners and inlining the source map will bloat out the already pretty big full version of p5.js.

that's a good call. i'll disable those :)

If I understand correctly grunt-terser is just to minify p5.dom.js? Just asking to confirm, no problem for me if that's the case.

yep!

Another detail, does the build step needs to be separate out into two? (build and build-p5)

Is it a stylistic choice for clarity or does separating the steps have some functional purpose? Again, no major opinion about this, just curious.

it was easier to reuse some logic/config, but i'm happy to make it into one step. that probably makes more sense since we don't really do one without the other, right?


i noticed that while es6 code in p5.js is transpiled into es5 (eg. const -> var), this is not the case for code in some of the 3rd-party libraries (search for const in p5.min.js in this branch).

@Spongman that's a really good catch, thank you. i'm looking into this now and will report back soon. until this is resolved we can't move forward with parcel here.

@lmccart
Copy link
Member

lmccart commented Mar 24, 2019

Thanks @outofambit this is looking great! I agree with @limzykenneth that it makes sense to combine build and build-p5. Once that's updated and the issue @Spongman found is resolved, I'm good with merging this.

@Spongman
Copy link
Contributor

I don’t think we can use parcel: parcel-bundler/parcel#13

@outofambit
Copy link
Contributor Author

Yeah, @Spongman i think you are right, and thank you again for catching this. There are open issues for this in parcel (parcel-bundler/parcel#1655, parcel-bundler/parcel#2500), but there has been little movement on them.

Weirdly, the parcel docs state "Parcel transpiles your code with @babel/preset-env by default, this is to transpile every module both internal (local requires) and external (node_modules) to match the defined target." (I'll probably open a PR to change that so others don't end up in this situation, too.)

Unfortunately, I think this means we're going to have pass on parcel. I've tried a number of workarounds, but none have worked. Apologies to all involved that this didn't pan out. 😞

@outofambit outofambit closed this Mar 25, 2019
@Spongman
Copy link
Contributor

that's a real shame. if it wasn't for this, it seems like this would have been a good solution.

@hunterloftis
Copy link

Just ran into this too. It's a real pity that (imo) makes parcel essentially unusable. What's the point of transpiling the top of the iceberg when all the rest is going to break in many users' browsers?

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

Successfully merging this pull request may close these issues.

5 participants