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

[WIP] es6 with babel/generator-babel-boilerplate #968

Merged
merged 64 commits into from Oct 29, 2015
Merged

[WIP] es6 with babel/generator-babel-boilerplate #968

merged 64 commits into from Oct 29, 2015

Conversation

guillaumepotier
Copy link
Owner

ES6

Use es6 for Parsley development and build it fully transpiled into dist/. Should lead to 2.3.0 version which do not bring any new feature nor BC break.

TODO

  • es6 boilerplate (gulp/grunt tasts)
  • adapt Parsley files
  • adapt test suite
  • profit

@guillaumepotier
Copy link
Owner Author

@marcandre I've reached in this first WIP commit to build transpiled source into dist/ file but took very little time to adapt correctly files, I just avoided transpiling errors by adding import and export directives, but a lot are falsy.

I also noticed that since last week there is also a deprecation notice while transpiling with this stack. IMO this is not very important for now, we'll switch to esperanto or rollup to transpile later when change would be really needed.

Could you have a quick glance to this first commit and tell me if you think this is the right direction to take?

Thx a bunch

@marcandre
Copy link
Collaborator

Hi Guillaume, this is exciting.

Looks like the right direction for sure.

Not too sure what you mean by "adapt Parsley files", but I making the tests run is the next step. Then we can start having fun using the ES6 features.

@marcandre
Copy link
Collaborator

I restructured the commit, so that the diff can be understood. I edited my comment coz I was looking at the dist files, which shouldn't be in the commit anyways...

@guillaumepotier
Copy link
Owner Author

adapt Parsley files

I meant that it cannot work out of the box just by replacing defines by imports/exports. I do not handle all the es6 subtleties to make it work yet. We'll have to make some changes to work that way. Then yes, once working basically in the browser, I'll have to have a look to test suite.

Regarding transpiled files in dist/ folder, what do you mean? That is too early to put some here or that we should never commit them? If you mean the 2nd choice it can't be possible, we need transpiled commited javascript files there for bower or everyone that download parsley through a .zip release. Right?

I'll try to have a look this week end, but not 100% sure, it's back to school here and got a lot to do :(

Best

@marcandre
Copy link
Collaborator

I meant that it cannot work out of the box just by replacing defines by imports/exports. I do not handle all the es6 subtleties to make it work yet.

You're sure? I didn't check, but I assumed it should work fine. ES3/5 is valid ES6.

Regarding transpiled files in dist/ folder, what do you mean? That is too early to put some here or that we should never commit them?

I'd say that commits should only touch src/ (and config, etc...) except for release commits that should only touch dist/ (and possibly Changelog/doc). No mixing of both.

@guillaumepotier
Copy link
Owner Author

You're sure? I didn't check, but I assumed it should work fine. ES3/5 is valid ES6.

Yes but I suspect requirejs behaves not exactly as an import/export does. I think it would need some adjustments

I'd say that commits should only touch src/ (and config, etc...) except for release commits that should only touch dist/ (and possibly Changelog/doc). No mixing of both.

Ok we are on the same page here. Sorry then for having comimtted the dist files now :s I'll ammend the commit

@marcandre
Copy link
Collaborator

Sorry then for having comimtted the dist files now :s I'll ammend the commit

No problem at all. I already did it. And split your commit in two, so conflicts are easier to fix.

@marcandre
Copy link
Collaborator

Changed paths to be relative. Now gulp test-browser fails to load only jQuery, not sure how/where to specify it...

I think the next step is to have the tests run.

// Return undefined if applied to non existing DOM element
if (!$(this).length) {
ParsleyUtils.warn('You must bind Parsley on an existing element.');
import $ from 'jQuery';

Choose a reason for hiding this comment

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

The string needs to be exactly the same as the name of the module that you're requiring in. In this case, it should be jquery (and not jQuery), as that's the name of the module that jQuery registers itself under on npm, Bower, and so on.

I haven't looked too thoroughly at this PR, but I would expect you to also need to add jquery as a dep in package.json. Atm it's only specified in the Bower package. The reason for this is that the behavior of the module bundler used by the Babel boilerplate effectively mimics a Node CommonJS module. In the same way that a Node script will search through node_modules to resolve packages, libs built with the boilerplate will do the same.

Lmk if this helps, or if you have other Q's!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thx for the quick feedback. I made the tweaks you pointed out, still won't load properly though... Note that the "test" is just the boilerplate test requiring our lib...

[~/parsley(es6)]$ gulp
[DEPRECATION NOTICE] Esperanto is no longer under active development. To convert an ES6 module to another format, consider using Babel (https://babeljs.io)
[22:28:08] Using gulpfile ~/parsley/gulpfile.js
[22:28:08] Starting 'lint-src'...
[22:28:08] Starting 'lint-test'...
[22:28:08] Finished 'lint-test' after 82 ms
[22:28:08] Finished 'lint-src' after 385 ms
[22:28:08] Starting 'test'...
[22:28:10] 'test' errored after 1.45 s
[22:28:10] Error in plugin 'gulp-mocha'
Message:
    Cannot find module 'jquery'
Details:
    code: MODULE_NOT_FOUND
Stack:
Error: Cannot find module 'jquery'
    at Function.Module._resolveFilename (module.js:336:15)
    at Function.Module._load (module.js:278:25)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/mal/parsley/src/parsley.js:12:15)
    at Module._compile (module.js:460:26)
    at normalLoader (/Users/mal/parsley/node_modules/babel-core/lib/api/register/node.js:199:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/mal/parsley/node_modules/babel-core/lib/api/register/node.js:216:7)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)

Any idea?

@jamesplease
Copy link

The build is currently failing because jQuery isn't required in a file that is trying to using it.

With ES6 modules, it's best to think of each file as existing in its own little universe. If pubsub.js uses jQuery, then it's gonna need to require it in itself via import $ from 'jquery';. There are a few ways around this, like attaching $ to the global namespace and accessing it globally, but those sorts of patterns defeat the purpose of using ES6 modules, I think.

@marcandre if you want to set aside some time to figure this out synchronously (google hangouts, gitter, IRC, etc.), let me know and we can schedule a meeting.

@marcandre
Copy link
Collaborator

Thanks for the feedback.
Right, I added the missing imports, but it didn't change the error (and it couldn't, really, as it talks about jquery, not $). But then I realized I didn't run npm install after changing package.json 😊 ... So now I'm getting a different error (yay!)

Message:
    jQuery requires a window with a document
Stack:
Error: jQuery requires a window with a document
    at module.exports (/Users/mal/parsley/node_modules/jquery/dist/jquery.js:29:12)
    at Object.<anonymous> (/Users/mal/parsley/src/parsley/pubsub.js:7:7)
    at Module._compile (module.js:460:26)
    at normalLoader (/Users/mal/parsley/node_modules/babel-core/lib/api/register/node.js:199:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/mal/parsley/node_modules/babel-core/lib/api/register/node.js:216:7)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/mal/parsley/src/parsley.js:15:47)

I guess I'll have a look later on, as a quick search didn't come up with an obvious solution, but of course if you happen to have a solution for me... 😄

@jamesplease
Copy link

This is likely because the tests are running in a Node environment, and not in a browser. jQuery doesn't Just Work© in Node. This is 'cause it's built for the browser: primarily as a wrapper for the DOM API. And Node doesn't implement the DOM.

There are at least two approaches you could take:

  • JSDom (quickest to run and set up)
  • Karma (allows you to test in real browsers!)

I usually use JSDom, though for bigger libraries and apps Karma is a fantastic tool. More specifically, I tend to use simple-jsdom, which is by far the easiest way I know of to get jQuery tests working on the server. The README explains it, but it's as simple as:

npm i simple-jsdom --save-dev

Then, somewhere in your test setup script, add this code:

require('simple-jsdom').install();

In ES6, that would be:

import simpleJSDom from 'simple-jsdom';
simpleJSDom.install();

You will need to do this before jQuery is loaded.

Hope that helps!

@marcandre
Copy link
Collaborator

Just awesome, things are working very well!

I'm adapting the tests and moving them from test/features to test/unit, so far so good.

Thanks again @jmeas <3 <3

@guillaumepotier
Copy link
Owner Author

Wow, this is great news! Good work guys! 💯

@jamesplease
Copy link

Just awesome, things are working very well!

Woo! Glad to hear it!

@guillaumepotier
Copy link
Owner Author

Also note that you may want to check browser compatibility differences. Babel compiles to ES5, so use of the polyfill + es5 shim is necessary by consumers of any Babel-ified library to support earlier browsers (and certain ES2015 features which are less commonly used)

This is a solid point. Parsley first aim was to be compatible with most browsers, IE8 for instance that we still support here @Wisembly :s

Could be great to specify browser compatibility for regular /dist/parsley.js file and maybe extended compatibility with shims.

@marcandre is test-suite still runnable on browser ? If so, I could check on browserstack on various browsers.

Best

@marcandre
Copy link
Collaborator

Indeed, forgot about the es5-shim, this is needed for IE8 only IIUC. So only provide link in Installation doc and Changelog should be ok.

IIUC, the polyfill won't be needed until we start using some of the features of ES6 (listed in the caveat list), so right now we're ok. When we need it, we should bundle it in dist/parsley.

Current setup to run in the browser is that gulp test-browser compiles the whole thing into tmp/__build.js or something. At that point the test/runner.html runs all locally, so should be easy to run on browserstack.

@jamesplease
Copy link

this is needed for IE8 only IIUC

Mhm.

IIUC, the polyfill won't be needed until we start using some of the features of ES6 (listed in the caveat list), so right now we're ok.

Yup, that's right.

When we need it, we should bundle it in dist/parsley.

This might cause some folks to bundle it twice, and if I remember correctly the shims together are not a negligible file size.

@marcandre
Copy link
Collaborator

Updated to doc, Changelog for dependency on es5-shim. Added a check in the code when loading Parsley too.
AFAIK, ready to merge...

@guillaumepotier
Copy link
Owner Author

Ok good for me 👍 (except maybe this weird unsquashed commit :p)

@guillaumepotier
Copy link
Owner Author

I'll let you do the merge, the push and the tag. I'll publish to npm once done.

@marcandre
Copy link
Collaborator

Merged in after final tweaks 😄 👯

@marcandre
Copy link
Collaborator

Just realized the Tests tab on the main site needs updating, so I'll do that asap. The gh-pages is the old one until then.

@jamesplease
Copy link

woooo! 🎉

@guillaumepotier
Copy link
Owner Author

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

@marcandre
Copy link
Collaborator

Cool, the tests tab now works, and master == gh-pages.
I added Parsley to the list of libs using the boilerplate :-)

@marcandre marcandre deleted the es6 branch October 30, 2015 19:57
@jamesplease
Copy link

@marcandre / @guillaumepotier how has this branch been working out for y'all? Any bugs or problems with the ES2015-ified build?

@marcandre
Copy link
Collaborator

No issue at all, works well.

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.

None yet

4 participants