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

AMD -> ES6 #678

Merged
merged 6 commits into from
May 7, 2014
Merged

AMD -> ES6 #678

merged 6 commits into from
May 7, 2014

Conversation

Rich-Harris
Copy link
Member

This pull request converts all the AMD modules in src/ to ES6 modules. The heavy lifting was done by recast but it turned to require a fair bit of manual cleaning up as well - so given that it will cause merge conflicts with any other work that's done concurrently, we should probably merge this in soon if we're going to merge it in at all, otherwise a lot of that work will have to be repeated.

Why ES6 modules?

  • much more compact syntax - no boilerplate, one less level of indentations
  • paves the way for us to use ES6 features like arrow functions
  • makes us look futuristic

Downsides?

Only one that I can see - in order to do tests (or use the sandbox folder) locally without building, there's an intermediate transpilation step required. It turns out that this is very quick, and can be triggered with grunt watch, so I don't think it's a dealbreaker. (We could always move away from grunt and towards something more sophisticated like broccoli, which is quicker still.)

So what does everyone think - 👍 or 👎?

@pateketrueke
Copy link

Sounds great, perhaps any js-project should evolve to be es6-compliant, so... 👍

@martypdx
Copy link
Contributor

martypdx commented May 6, 2014

I've got a checkin coming for refactoring of configuration/registries, probably about 10 files so easier for me to merge then for you to redo this.

Otherwise, sounds good. I was playing with broccoli couple of weeks ago. One nice feature is that it keeps a live server with build changes like ES6 and it's optimized for rebuild speed during . Once you get this up and running, I'll implement it as an experiment along side grunt for transpile. There was a broccoli node/windows issue, but appears to be close: broccolijs/broccoli#39. Are people using windows or mac for dev?

@aulizko
Copy link

aulizko commented May 6, 2014

ES6 for the win! ;)
Toward the future!

@silentworks
Copy link

@martypdx I am using Windows. And yeah I am happy with the ES6 merge too.

@browniefed
Copy link

I take it this is a 100% polyfill that will work for IE8 legacy version as well?

@Rich-Harris
Copy link
Member Author

Sounds like a consensus. Will merge it in so I'm not holding @martypdx up (sorry for the extra work Marty!)

@browniefed - yes, this doesn't actually affect the final result at all. The old AMD modules have been transpiled to ES6; during the build process they get transpiled back to ES5/AMD - apart from a bit of whitespace shifting around the end result is identical.

@Rich-Harris Rich-Harris merged commit c22ce99 into dev May 7, 2014
@Rich-Harris Rich-Harris deleted the es6ify branch May 7, 2014 00:11
@martypdx
Copy link
Contributor

martypdx commented May 8, 2014

Did you merge in changes to master before this?

@Rich-Harris
Copy link
Member Author

no, which changes?

@martypdx
Copy link
Contributor

martypdx commented May 8, 2014

As best I can tell, I think it's just the May 07 commits.

I am a git novice (or maybe just gitigornant), I thought the massive ES6 changes would make it an issue, but it looks like that's not the case.

@Rich-Harris
Copy link
Member Author

fingers crossed...

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

6 participants