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

chore: add a simple babel build step #327

Closed
wants to merge 2 commits into from
Closed

Conversation

trusktr
Copy link
Member

@trusktr trusktr commented Mar 19, 2017

Notes:

  • Source code currently remains mostly entirely the same, but replaced some vars with let or const to show that it is transpiled.
  • babel transpiles to dist folder.
  • Removed manually written UMD module footer, babel creates it now, we just module.exports = TWEEN.
  • Renamed src/Tween.js to src/TWEEN.js because babel's UMD output automatically sets the global to be named after the file name, so a filename of Tween.js means babel does global.Tween = mod.exports, while a filename of TWEEN.js makes babel do global.TWEEN = mod.exports.
  • The test script runs the build script first, and runs unit tests against dist/TWEEN.js. jshint and jscs still run against src/TWEEN.js.
  • The result still runs in whatever browsers were previously supported, because the output is currently the same (minus simple reformatting that babel does).

This is a small step towards updating to a modern code base while still supporting older browsers.

@trusktr
Copy link
Member Author

trusktr commented Mar 19, 2017

This works locally, but I'm not sure why Travis CI doesn't find the babel modules.

@trusktr
Copy link
Member Author

trusktr commented Mar 19, 2017

Aha! I tried using npm@^2 like Travis CI is doing, and that reproduces the error too.

Is there some way to update NPM on Travis? v2 is rather outdated, as we're already on v4 which has lots of benefits.

@trusktr
Copy link
Member Author

trusktr commented Mar 19, 2017

Oops, I had forgotten to use --save-dev when I installed babel-preset-env, that's why.

All green now!

@trusktr
Copy link
Member Author

trusktr commented Mar 19, 2017

@sole @dalisoft @mikebolt thoughts?

@trusktr trusktr mentioned this pull request Mar 19, 2017
@trusktr
Copy link
Member Author

trusktr commented Mar 19, 2017

I tested in local webpack and browserify projects and it works in both. It just outputs UMD, which should basically work everywhere.

@mikebolt
Copy link
Contributor

I am not a big fan of this, but I guess it makes the code safer.

I am worried that we will try to turn this into an es6 version and we will end up with two es6 versions of tween.js. Maybe I don't understand completely. Are we going to use anything other than "let" and "const" and maybe arrow functions?

@mikebolt
Copy link
Contributor

We will need a README update with new build instructions.

@trusktr
Copy link
Member Author

trusktr commented Mar 21, 2017

IMO the best approach would be to have a single repo (this one), and make gradual changes to this repo towards ES6. We could take ideas from the es6-tween repo and apply them here over time, rather than having two repos (and two NPM packages), and rather than applying a huge mondo commit to this repo. Three.js doesn't have two repo or two NPM packages, for example.

Thoughts on this idea? @mikebolt @d

This pull request was the first, simplest change towards that direction, while keeping everything working.

Let me add the README update. I think semantic-release runs the test script, and the test script happens to build everything first in order to test output files, so I.. think the workflow is exactly the same as before. Let's see what sole says.

@mikebolt
Copy link
Contributor

mikebolt commented Mar 21, 2017 via email

@trusktr
Copy link
Member Author

trusktr commented Mar 21, 2017

That's the whole point of this change.

Do you really want to maintain different code bases of the same library? That doesn't seem ideal if it can be avoided. I wouldn't want to propose a change and then have to go write a second version of the change in for other code base; that doesn't sound as productive as having one code.

We can maintain a single code base that is transpiled into dist as es5 (or es3 if we want), and we can check the dist folder into the repo if we want to prevent users from having to run build steps. This would be more productive than spending time writing two versions of every change.

@trusktr
Copy link
Member Author

trusktr commented Mar 21, 2017

Actually, unpkg.com already allows using NPM as a CDN, so we can add that to this README (like es6-tween has), and ES5 users can use that just fine.

@trusktr
Copy link
Member Author

trusktr commented Mar 22, 2017

@mikebolt I like #331 more, so closing this one. I think it suits the project better than Babel. I hadn't seen Bublé before.

I still think either PR is a better start than maintaining two code bases.

@trusktr trusktr closed this Mar 22, 2017
@trusktr
Copy link
Member Author

trusktr commented Mar 22, 2017

Plus if you look at Bublés output, it is much much more like ES5 than Babel, I think you'll like it...

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