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 and commonjs to es6 #131

Closed
wants to merge 2 commits into from
Closed

Amd and commonjs to es6 #131

wants to merge 2 commits into from

Conversation

vmora
Copy link
Contributor

@vmora vmora commented Jul 6, 2016

This is #44 again.

vmora added 2 commits July 6, 2016 16:12
```sh
sed -i "s|define.'[^']*',|define(|g" $(git grep --name-only  -e
"define[(]'[^']*',")

npm install amd-to-es6
./node_modules/.bin/amdtoes6 -d src/ -o src/ -b

npm install esnext
find src/ -type f -name "*.js" \
    -not -name "*.min.js" -not -name "optimer_regular.js" \
    -exec ./node_modules/.bin/esnext -I -w modules.commonjs {} \;
```
@@ -33,7 +33,7 @@ <h1 class="page-title">Source: ApiGlobe.js</h1>
*/


define('Core/Commander/Interfaces/ApiInterface/ApiGlobe', [
define( [
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this define needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an artifact of the sed script being used to transform "named defines" to "unnamed defines" prior to running amdtoes6 (which doesn't support named defines). The sed script modified that file, but amdtoes6 obviously didn't.

IMO, either we:

  • ignore it, leave the commit like that; we'll eventually remove the generated doc from master anyway
  • remove doc from master first, then rebase that change on top
  • exclude that file from the commit

@vmora
Copy link
Contributor Author

vmora commented Jul 7, 2016

There is a need for decision here, since we have mixed AMD and commonjs imports.

Here's my vote:

  1. es6 import, since it's the fututre
  2. commonjs, good enought and easy to transform afterward
  3. AMD, last choice for me since it's error prone (2 lists that should be in the same order... but sometimes aren't)

ping @iTowns/core-devs

@@ -29,6 +30,12 @@
"simd": "^2.0.0"
},
"devDependencies": {
"babel-cli": "^6.5.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really not sure babel-cli is needed.

AFAICT, the babel executable is installed as part of babel-core, and babel-cli is meant to be installed globally (npm install -g babel-cli) so it's available on your command-line anywhere, and will "sniff" the current module to actually call the locally-installed babel (at node_modules/.bin/babel) if it exists; defaulting to the globally-installed one otherwise.

@vpicavet
Copy link
Contributor

May be related : mrdoob/three.js#9310

@peppsac
Copy link
Contributor

peppsac commented Jul 22, 2016

May be related : mrdoob/three.js#9310

This has been merged. I think it's a great example to follow for itowns.

I'm going to work on this done soon, unless:

  • someone thinks it's a bad ida.
  • someone else want to do it :)

@iTowns/reviewers : thoughts?

@tbroyer
Copy link
Contributor

tbroyer commented Jul 22, 2016

I just updated #44 with code that appears to work (using npm start)

I removed everything somehow related to publishing the module on the NPM registry (e.g. transforming ES6 to ES5 before publishing)

@vmora
Copy link
Contributor Author

vmora commented Jul 25, 2016

thoughts?

+1

@peppsac
Copy link
Contributor

peppsac commented Jul 25, 2016

@tbroyer PR #44 's branch seems outdated?

@tbroyer
Copy link
Contributor

tbroyer commented Jul 25, 2016

Hmm, amdtoes6 branch has been updated, but the PR being closed it wasn't updated. Let me create a new PR.

@tbroyer
Copy link
Contributor

tbroyer commented Jul 25, 2016

#169

@peppsac
Copy link
Contributor

peppsac commented Jul 27, 2016

PR #169 is now merged, we can close this one - thanks @vmora and @vpicavet for your help :)

@peppsac peppsac closed this Jul 27, 2016
@peppsac peppsac deleted the amd_and_commonjs_to_es6 branch July 27, 2016 09:52
@vmora
Copy link
Contributor Author

vmora commented Jul 27, 2016

@peppsac, @tbroyer thanks.

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.

4 participants