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

[Suggestion]: Adding webpack to allow ES201* JavaScript #202

Closed
tdeekens opened this issue Mar 6, 2017 · 16 comments · Fixed by #206
Closed

[Suggestion]: Adding webpack to allow ES201* JavaScript #202

tdeekens opened this issue Mar 6, 2017 · 16 comments · Fixed by #206

Comments

@tdeekens
Copy link
Collaborator

tdeekens commented Mar 6, 2017

This issue proposes a feature which adds webpack as a build tool.

Background & Context

Currently DOMPurity does not have a built tool in place. This is nice and often simplifies things. However, adding webpack might give the project some benefits and this issue is about to align on the potential gain of it.

Feature

Adding webpack would yield the following benefits

  • Simpler modularisation of code (constants, config and logic)
  • Writing more "modern" JS with ES201*
  • Removes the "clutter" for universal JS at the beginning of src/purify.js
    • Webpack adds this to the dist
  • Removes "custom" minification as webpack has different "targets"
    • Uses Uglify too

Adding webpack might have the following disadvantages

  • A built tool always adds complexity in mental overhead and approachability to a project
  • The test setup needs to be adjusted to consume a dist file

What do you all think? Might this be worth to at least spin up a PR for and discuss progress along the way or is it something DOMPurify does not really need? I would be able to drive the change...

@cure53
Copy link
Owner

cure53 commented Mar 7, 2017

👍

@tdeekens
Copy link
Collaborator Author

tdeekens commented Mar 7, 2017

I love you 😂

@cure53
Copy link
Owner

cure53 commented Mar 7, 2017

What are steps that we can do here? How can I help?

@tdeekens
Copy link
Collaborator Author

tdeekens commented Mar 7, 2017

...just to make sure: I wasn't disappointed by the simple 👍🏾 or anything just enjoyed the simplicity of it.

Draft of steps would be

  1. Add webpack config (according to the survivejs cookbook)
  2. Slice up the code into "meaningful" modules
    • This is something where I think your input would be ace. I'll do a first sketch and you comment along...
  3. Refactor ES5 to some ES6 using some syntax sugar
  4. Integrate build process into npm-scripts etc

Something we could try along the way as a final step is to remove the dist files from the repo. We could add them right before the npm publish so they would be on npm but not inside the repo. The small example could then use https://unpkg.com/[email protected] so spin up examples etc.

Makes sense?

@cure53
Copy link
Owner

cure53 commented Mar 7, 2017

From my side no objections.

However, let's hear what the other core contributers have to say: @neilj and @fhemberger - any objections?

@tdeekens
Copy link
Collaborator Author

...wondering if they have any objections

@fhemberger
Copy link
Contributor

Seems fine to me. I'm curious in how far the size of the dist js may differ afterwards.
Just give it a shot!

@neilj
Copy link
Contributor

neilj commented Mar 14, 2017

In general this seems fine. My main concern would be whether you include the final build file in the repo or not. In general, you don't want to store build products in version control (for a start, you have to remember to keep it in sync), but if you don't then every user now has to be able to set up the build environment in order to use it, whereas at the moment they can just clone the repo and copy over the single file they need. So I'd probably advocate for including the single ES5 file version still in the repo.

@tdeekens
Copy link
Collaborator Author

@neilj totally agreed. The distribution files do not belong into the repo. With keeping the end user in mind this might be troublesome for him though. However, I think I failed to point out how to circumvent the problem:

  1. DOMPurity is hosted on n CDNs where it is automatically updated on a push to master
    • People can grab it there and we have a link to "right click save" on the readme
  2. The dist files get added on the "preversion" nom lifecycle hook
    • They won't be on GitHub but on an npm install and available through unpkg.com

Combing both I think we would end up at a sweet spot for both sides.

@cure53
Copy link
Owner

cure53 commented Mar 14, 2017

So I'd probably advocate for including the single ES5 file version still in the repo.

Me too, This is critically important.

@cure53
Copy link
Owner

cure53 commented May 14, 2017

Hi all, any news here? This has been stale for a while.

@tdeekens
Copy link
Collaborator Author

tdeekens commented May 14, 2017

Sorry for letting it stale. I didn't have time taking this further but will as soon as possible. The main issue is still that we use the factory function from our UMD build to re-export the library. This will not be possible when bundling works through webpack which will itself surround or library with the UMD export logic not giving access to the factory function.

This all relates to #206.

@cure53
Copy link
Owner

cure53 commented May 14, 2017

Same here no worries :D

@jfparadis
Copy link

@tdeekens, to support ES6, could I suggest using Rollup instead of Webpack?

Webpack adds a lot of boiler plate code, and is better suited to assemble web apps where code splitting is crucial for load performance. That is not required to build a library, and Rollup will assemble code into a more integrated and run-time efficient unit.

The mantra is:

Use webpack for apps, and Rollup for libraries.

Rollup was created for a different reason: to build flat distributables of JavaScript libraries as efficiently as possible, taking advantage of the ingenious design of ES2015 modules. Other module bundlers — webpack included — work by wrapping each module in a function, putting them in a bundle with a browser-friendly implementation of require, and evaluating them one-by-one. That’s great if you need things like on-demand loading, but otherwise it’s a bit of a waste, and it gets worse if you have lots of modules.

https://medium.com/webpack/webpack-and-rollup-the-same-but-different-a41ad427058c

@tdeekens
Copy link
Collaborator Author

I agree with the article and general mantra. I however don't feel adding webpack or rollup is the bulk of the work in this case. If you follow the PR on the matter it's also replacing jshint with eslint, while adding prettier as a config base and formatter, migrating a bit of library code while most importantly replacing the UMD closure reliance of the library. I am happy to play and evaluate rollup vs. webpack as a tool in the end.

@tdeekens
Copy link
Collaborator Author

Alright, @jfparadis the updated bundling process in #206 uses rollup now. What do you think of the setup?

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 a pull request may close this issue.

5 participants