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

Implements Tests and rewrite the codebase using ES6 (related to the issues #33 #3) #34

Closed
wants to merge 23 commits into from

Conversation

thiamsantos
Copy link
Contributor

coverage

  • Rewrite the codebase using ES6.
  • Add a build step for bundling three different versions of the module using rollup. A CommonJS version is expose through the property main on the package.json, an ES6 version is exposed through the property module on the package.json and also a umd version will be available at unpkg after this PR was accepted and ublished to npm.
  • Add node gitignore.
  • Add a visual test example to see waterfall in action. Was used the same example available at the project page.
  • Add git hooks with a help from husky to:
    1. Prevent commits that doesn't follow the code style.
    2. Always build the files before commiting.
    3. Prevent commits that have bronken tests.
  • Add a npm script to run the build, test and the visual at watch mode. You can just type npm start.

@thiamsantos thiamsantos changed the title Implements Tests and rewrite the codebase using ES6 #33 #3 Implements Tests and rewrite the codebase using ES6 (related to the issues #33 #3) Feb 9, 2017
@raphamorim
Copy link
Owner

Ref #34 #33
Nice @thiamsantos, I'll review it tonight 👍

@thiamsantos
Copy link
Contributor Author

Any thoughts @raphamorim?

@raphamorim
Copy link
Owner

I'm start reviewing it now, sorry by delay

I have some questions to proceed:

  • Why to use ES6? (Only to know)
  • Why it generate two files: cjs and umd version? Why it? How bower will understand it?
  • Can you add Nodejs 7 in travis.yml?
  • If you're already using ES6 notation, can you test import cases too?

Also nice job testing UI behavior and remaining waterfall size after transpile it (I believe Rollup was a good choice)

@thiamsantos
Copy link
Contributor Author

thiamsantos commented Feb 17, 2017

Thanks for taking time to review it @raphamorim!!

Why to use ES6?

ES6 provide some improvements on the syntax. Since a refactor was needed to implement the unit tests, why not use ES6 in that refactor?

Why it generate two files: cjs and umd version? Why it? How bower will understand it?

The cjs version is handled by npm when you type require('waterfall.js'). And the UMD version is used when you just embed the script on the browser (maybe using unpkg). For bower is needed to specify the main property on bower.json pointing to the UMD version (I forgot to do that 😁).

Can you add Nodejs 7 in travis.yml?

Sure. I will add the latest stable Node.js release to travis.yml.

If you're already using ES6 notation, can you test import cases too?

The import statement was not working just because the main property on package.json was pointing to a file that did not exist.

@thiamsantos
Copy link
Contributor Author

On second thought the cjs version is actually not necessary, because the umd version is 'universal' and works well with CommonJS modules. So I can remove the cjs a leave just the umd version and the ES6 version. What do you think @raphamorim? I can commit that?

@thiamsantos
Copy link
Contributor Author

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Seu código é muito bonito, @thiamsantos 😄 👍

@thiamsantos
Copy link
Contributor Author

Any thoughts @raphamorim?

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.

2 participants