Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

solves #67: massive refactor to use ES6 modules under the hood #74

Merged
merged 7 commits into from
Sep 12, 2014

Conversation

caridy
Copy link
Collaborator

@caridy caridy commented Sep 5, 2014

complete refactor to align with all other intl-* projects that are also using ES6 under the hood.

here is the list of things (from the top of my head):

  • dist/ folder now host the distro for browsers
  • locale-date/ remains the same, holding the computed data from cldr, including a new complete.js used to produce dist/Intl.complete.js
  • src/ is the source of true, where you put your ES6 modules.
  • src/core.js is the former Intl.js from root
  • src/exp.js is an example of how to extract pieces from core into its own module (hopefully we will have more and more of those in the future, since that's the whole point of this PR)
  • src/main.js is for browsers only, a trick to expose the polyfill as global Intl when there is no Intl.
  • index.js is the entry point for nodejs when people do require('intl'), and it does the detection, it expose Intl as global when needed and set the complete locale data in memory as well.
  • lib/ folder is a 1-1 mapping of src/ for commonjs/nodejs, and index.js uses it when you require intl.

Other considerations to keep in mind:

  • dist/ folder is committed so we can use those thru bower :/
  • I haven't test the output in a browser, but it does work in node.js
  • the entry in package.json called jsnext:main is a new feature that we are promoting, and it is what the grunt task uses to transpile the ES6 modules (we can talk more about that)
  • I'm proposing to make this 1.0.0 since the code is stable (I didn't change anything in the logic of the polyfill)

Development:

I tried to keep everything the same:

  • no more npm run build.
  • grunt cldr and it produces the locale-data folder, it does not touch anything outside of the folder from now on.
  • no more npm run lint, npm test does that now. grunt jshint is also available.
  • grunt alone does the build process, which includes cleaning, linting and building dist/ and lib/ folder.
  • npm test does not build cldr, nor the dist/ and 'lib/` folder, just like it was before, but it now runs a clean up process and linting.

alright, makes sense? happy hacking!

@caridy
Copy link
Collaborator Author

caridy commented Sep 5, 2014

trying to fulfill my promise #67

this is a massive PR, please review @ericf @drewfish @andyearnshaw

@caridy
Copy link
Collaborator Author

caridy commented Sep 5, 2014

travis is failing due to saucelab for what I can tell, need a hand on that!

@ericf
Copy link
Collaborator

ericf commented Sep 5, 2014

travis is failing due to saucelab for what I can tell, need a hand on that!

Saucelabs + Travis doesn't work on PRs because of the credential issues.

@@ -12,3 +12,7 @@ cldr/

# NPM packages installed locally
node_modules

lib/
node_modules/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate entry.

// providing an idiomatic api for the nodejs version of this module
module.exports = exports = IntlPolyfill;
// preserving the original api in case another module is relying on that
exports.default = IntlPolyfill;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to use O.dP here to make this non-enumerable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

are we doing that in the other pkgs? I think IntlPolyfill is a controlled object, having default as a member will not hurt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are in one of them. It's worth looking at the ES6 spec. to see if default is spec'd to be enumerable. But the non-writable part makes sense to me for this.

@caridy
Copy link
Collaborator Author

caridy commented Sep 11, 2014

@andyearnshaw did you get a chance to look at this?

@andyearnshaw
Copy link
Owner

Not yet, hopefully tonight but if not then definitely tomorrow evening.
On 11 Sep 2014 03:37, "Caridy Patino" notifications@github.com wrote:

@andyearnshaw https://github.com/andyearnshaw did you get a chance to
look at this?


Reply to this email directly or view it on GitHub
#74 (comment).

@caridy
Copy link
Collaborator Author

caridy commented Sep 11, 2014

Cool, our priority at this point is to get this merge to do another refactor to separate number and datetime polyfills into its own pieces in case we want to load only one of them.

@ericf
Copy link
Collaborator

ericf commented Sep 11, 2014

Cool, our priority at this point is to get this merge to do another refactor to separate number and datetime polyfills into its own pieces in case we want to load only one of them.

This seems like it will greatly complicate the story for people wanting to use this polyfill. Why would we be concerned with separating these two pieces out when the rest of the Intl related libraries we're creating depend on both being there?

andyearnshaw added a commit that referenced this pull request Sep 12, 2014
solves #67: massive refactor to use ES6 modules under the hood
@andyearnshaw andyearnshaw merged commit f492978 into andyearnshaw:master Sep 12, 2014
@andyearnshaw
Copy link
Owner

Thanks @caridy, it all looks pretty good. The Sauce Connect tests are broken, but they were failing in some browsers anyway so it hasn't really affected the build status. As soon as we can get the tests fixed I'll update the releases.

This seems like it will greatly complicate the story for people wanting to use this polyfill. Why would we be concerned with separating these two pieces out when the rest of the Intl related libraries we're creating depend on both being there?

@ericf: I'm not so sure. I've used only the NumberFormat portion of Intl.js in one project already, which meant slicing up the source code and changing the data compiler to drop all Date stuff. If it were an option in the build process (ie ignore the DateTimeFormat import, recompile the data), it would have made things simpler.

@caridy caridy deleted the es6 branch September 12, 2014 18:46
@caridy
Copy link
Collaborator Author

caridy commented Sep 12, 2014

thanks for getting this in.

@ericf remember the conversation with have with mail, they probably want to pull in only NumberFormat.

@ericf
Copy link
Collaborator

ericf commented Sep 13, 2014

Okay, but we should be sure that the standard was of using this polyfill is that you get both constructors, and consider it advanced usage if you want to only use say NumberFormat.

@caridy
Copy link
Collaborator Author

caridy commented Sep 13, 2014

yeah, I'm just thinking about having multiple distributions of this for browsers, for the server it is not a problem.

@ericf
Copy link
Collaborator

ericf commented Sep 13, 2014

yeah, I'm just thinking about having multiple distributions of this for browsers, for the server it is not a problem.

@caridy okay, sounds good.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants