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

ES module for webpack tree shaking #1015

Merged
merged 13 commits into from
Nov 21, 2019

Conversation

banjankri
Copy link
Contributor

Tested against webpack 4.28.x
Fixes #814

@banjankri
Copy link
Contributor Author

Following a common approach, taken by ramda for instance: https://github.com/ramda/ramda/. They compile multiple flavors of their library to distribute. Please let me know if different path is envisioned.

@profnandaa
Copy link
Member

@banjankri - thanks for taking on this old issue. Quite a huge PR, let me review it over the weekend 👍

package.json Outdated
@@ -1,7 +1,7 @@
{
"name": "validator",
"description": "String validation and sanitization",
"version": "10.13.0",
"version": "10.12.0",
Copy link
Member

Choose a reason for hiding this comment

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

You can un-stage this change, version change is done by @chriso during publishing, since many other changes will be in by then.

@@ -0,0 +1,156 @@
import toDate from './lib/toDate';
Copy link
Member

Choose a reason for hiding this comment

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

Since we are having to repeat all the code in the /es folder, it means changes have to be made in duplicate. Could we think of a build process where this code is generated into the folder and all the necessary transformations made? 🤔

Copy link
Contributor Author

@banjankri banjankri Apr 15, 2019

Choose a reason for hiding this comment

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

This is done already, please see "build:es" script in package.json. It is then invoked as part of the "build" script. Added also cleanup for it accordingly, following what was done for "clean:browser" and "clean:node".

Copy link
Member

Choose a reason for hiding this comment

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

Oh, got it 👍

@profnandaa
Copy link
Member

@chriso -- could you check out this PR...

@banjankri
Copy link
Contributor Author

hey @chriso @profnandaa , just pinging to get the PR reviewed please :)

@profnandaa
Copy link
Member

Because it was introducing a huge diff, I thought @chriso will be best to take on this...

@banjankri
Copy link
Contributor Author

@chriso would you maybe have some time to go through this PR please?

@chriso
Copy link
Collaborator

chriso commented Oct 27, 2019

@banjankri sorry for the delay in reviewing this. I agree with the general approach and would be happy to merge – we just need it to be rebased to pull in the latest changes.

@banjankri
Copy link
Contributor Author

Merged with upstream master. Can see there is a non-empty diff on validator.js, which I would not expect, @profnandaa @chriso could you check if that is due to merge please?

@chriso
Copy link
Collaborator

chriso commented Nov 21, 2019

Merged with upstream master. Can see there is a non-empty diff on validator.js, which I would not expect, @profnandaa @chriso could you check if that is due to merge please?

@banjankri fixed by fc3f577.

@banjankri
Copy link
Contributor Author

@chriso pulled in the changes and merged again

@chriso
Copy link
Collaborator

chriso commented Nov 21, 2019

Looks good @banjankri. Thanks for your contribution! 😄

@chriso chriso merged commit c9e411c into validatorjs:master Nov 21, 2019
@chriso
Copy link
Collaborator

chriso commented Nov 21, 2019

This is available in 12.1.0

@kachkaev
Copy link

kachkaev commented Jan 8, 2020

Sorry for a stupid question. Do I understand correctly that I can do import { isBlah } from "validator" and expect all other methods to be excluded from the app bundle? Library typings don't seem to support that.

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.

Not tree shaking with webpack 4
4 participants