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

Migrate to ES6 #496

Merged
merged 41 commits into from
Feb 21, 2016
Merged

Migrate to ES6 #496

merged 41 commits into from
Feb 21, 2016

Conversation

forabi
Copy link
Contributor

@forabi forabi commented Feb 11, 2016

This is a large PR with some opinionated changes, so I'm not sure if it would be accepted, but I'd try anyway :)

  • Deprecate non-string coercion (major version upgrade): it is difficult to implement this functionality when each function is used separately.
  • Rewrite validation functions as ES6 modules, so we can now require each function separately without having to include the whole library, this will be helpful to anyone using validator on the client side, and will make it possible in the future to use tree shaking (i.e. removing unused code) when Node supports ES6 modules.

Functions are available on the root folder for convenience Functions are available in the lib/ dir:

// ES5
var isEmail = require('validator/lib/isEmail');

// ES6
import isEmail from 'validator/lib/isEmail';

// ES6 with tree shaking
import { isEmail } from 'validator';

Of course you can still include the entire library as usual:

var validator = require('validator');
validator.isEmail('[email protected]'); // => true

Now to the opinionated changes:

  • Move to ESLint: it is extensible, has better support for ES6 and has awesome integration extensions for popular code editors.

  • Use npm scripts to build, clean and test the package as they are more compatible with non-*nix platforms and easier to use.

    npm run clean # removes all builds
    npm run clean:browser
    npm run clean:node
    npm run build
    npm run build:browser
    npm run build:node
    npm run lint # runs ESLint
    npm run test

* Move each validation function to its own module, so you can now require only the functions you need e.g. require('validator/isEmail') instead of bundling the whole library in your scripts.
* Use babel to build for node
* Use rollup to generate an optimized browser bundle in umd format
* Use npm scripts instead of make.
* define is a giant hack, looks like it will handle everything you throw
at it
* Here is the implementation https://github.com/jrburke/requirejs/blob/a1da39714bc3eea9ab85c77e47d544e815e2699f/require.js#L2049
@chriso
Copy link
Collaborator

chriso commented Feb 12, 2016

Thanks for the PR! ✨ This will obviously take some time to review but I'm liking everything so far.

I'm happy to move from jshint to eslint, and also happy to drop the makefile in favour of npm scripts.

Dropping coercion of non-string input is already planned for v5, although I'd like for the deprecation notice to be out there in the wild for a bit longer, as there are undoubtedly projects and libraries that depend on the latest version of the library rather than ^4

@forabi
Copy link
Contributor Author

forabi commented Feb 12, 2016

Great!
I've added a new function assertString that is called by every validation function and currently throws an error if the input is not a string, toString is still available but it should be called manually. We could print the deprecation notice in assertString and throw the error, or even restore the coercion behavior by simply calling toString inside assertString.

@forabi
Copy link
Contributor Author

forabi commented Feb 12, 2016

Just wanted to point out that accessing individual functions using validator/<functionName> is not currently implemented, instead one has to use validator/lib/<functionName>. I'm trying to figure out the best way to do it without polluting the root dir with too many files. We may need to add tests for these files too.

@tunnckoCore
Copy link

I'm trying to figure out the best way to do it without polluting the root dir with too many files

Try export-files, set just one index.js in lib :)

* build no longer runs tests or cleans broken builds
* add clean:browser and clean:node
* add minify runs uglifyjs to generate validator.min.js
* add pretest (alias to build) which is run automatically before tests
* build-browser => build:browser
* build-node => build:node
* build:browser now eliminates dead code for unminified build
@chriso
Copy link
Collaborator

chriso commented Feb 13, 2016

The point of the deprecation notice was that it still coerced input but warned when it did so, giving you time to update your code. Printing a deprecation notice and then throwing an error defeats the purpose.

I'm happy for you to drop the deprecation notice entirely. Once this PR Is merged I'll tag and publish v5.

"tests",
"Makefile",
"package.json"
"Makefile"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Makefile no longer exists.

@forabi
Copy link
Contributor Author

forabi commented Feb 16, 2016

@chriso Tests are linted now. I've also added:

  • isMobilePhone validation for ar-SY (Arabic (Syria))
  • isAlpha and isAlphanumeric validation for various Arabic locales.

@forabi
Copy link
Contributor Author

forabi commented Feb 21, 2016

@chriso upgraded to ESLint 2 👍

@chriso
Copy link
Collaborator

chriso commented Feb 21, 2016

👍 we'll need to merge in the changes from master in the past week and then I'll look at merging this PR.

@forabi
Copy link
Contributor Author

forabi commented Feb 21, 2016

Do you want me to do that?

@chriso
Copy link
Collaborator

chriso commented Feb 21, 2016

If you don't mind. It won't let me merge otherwise.

@forabi
Copy link
Contributor Author

forabi commented Feb 21, 2016

@chriso Done!

@chriso
Copy link
Collaborator

chriso commented Feb 21, 2016

Apart from the README changes, this is ready to merge ✨

chriso added a commit that referenced this pull request Feb 21, 2016
@chriso chriso merged commit 396f2be into validatorjs:master Feb 21, 2016
@chriso
Copy link
Collaborator

chriso commented Feb 21, 2016

v5 will land shortly.

@forabi
Copy link
Contributor Author

forabi commented Feb 21, 2016

Awesome! 😄
Thank you!

@chriso
Copy link
Collaborator

chriso commented Feb 21, 2016

5.0.0 has been published. Thanks for the PR!

rustybailey added a commit to express-validator/express-validator that referenced this pull request Mar 13, 2016
…ce it doesn't automatically coerce non-strings to strings anymore: validatorjs/validator.js#496
@ldong
Copy link

ldong commented Nov 1, 2016

I dont understand why it does not work for Ember by default? Something like:

import validator from 'bower_components/validator-js';

I had to do this

bower install validator-js --save-dev

Then inside ember-cli-build.js, add this line of code app.import('bower_components/validator-js/validator.js');

@chriso Thanks for the help.

TigerStar429 pushed a commit to TigerStar429/expressjs-validator that referenced this pull request May 30, 2023
…ce it doesn't automatically coerce non-strings to strings anymore: validatorjs/validator.js#496
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.

5 participants