cldrjs icon indicating copy to clipboard operation
cldrjs copied to clipboard

export name

Open ichernev opened this issue 10 years ago • 14 comments

It looks like the export name under AMD is cldr, under CommonJS is cldr.js and the global variable is named Cldr. Can it be a bit more consistent?

ichernev avatar Mar 02 '14 00:03 ichernev

It could be improved, any suggestion? :p

As far as I know, the export name on AMD or CommonJS depends on the user's environment. On AMD, it depends how users configure paths. On CommonJS, it depends how users name directories.

This project has been registered under the cldr.js name on both package managers: bower and npm. So, it's installed on bower_components/cldr.js using bower install cldr.js, or node_modules/cldr.js using npm install cldr.js.

My node.js example suggests using npm install cldr.js, therefore the exports name is cldr.js: Cldr = require( "cldr.js" );.

My require.js example suggests using bower install cldr.js, and configuring path cldr: "bower_components/cldr.js/dist/cldr", therefore the exports name is cldr: define([ "cldr" ], function( Cldr ) {...}).

Should we suggest users to define path "cldr.js": "..." on require.js?

I agree the consistency among the export names could be improved, I've caught myself with the same concern once, looking for ideas.

rxaviers avatar Mar 06 '14 14:03 rxaviers

I guess the .js is a bit redundant. In moment.js we try to export moment as a name (moment.js was also an available package, but is now deprecated). That fixes most of it. For the capital letter -- is this to distinguish between the root object, and a specific locale? In any case it can be lower case. Upper case functions imply constructors, and yours is just a function (not invoked with new), so yet another reason :)

ichernev avatar Mar 06 '14 16:03 ichernev

I guess the .js is a bit redundant.

Do you mean cldr for npm and bower? If so, it's unfortunately not an option, because both have been registered by different projects before this one.

For the capital letter...

Cldr is a class. It's not a function. On README, note the difference between the Cldr.s and cldr.. The former are class methods, the latter are instance methods.

rxaviers avatar Mar 06 '14 17:03 rxaviers

Yeah, my bad. It is a constructor. Most projects that I've worked with try to hide the constructor and instead provide a factory function. It is a simpler interface (users don't have to remember when to do new and when not), and you can provide multiple constructors (factories) instead of relying on the single one exported by your module. You can also change the implementation to use another object pattern (closures for example).

About the .js -- project names differing only by the .js also doesn't sound great. But the other option is changing the project name which is your call.

ichernev avatar Mar 06 '14 19:03 ichernev

No problem. About the factory function, we can discuss that in another issue if that turns out to be a problem.

About the name, I am not sure what other name to use. But, I think for now (beginning phase), let's make sure we build a great product, and we keep our goal: simple layer to facilitate I18n softwares to access and use the official CLDR JSON data. If our name becomes a problem, we can always fix that. I still think consistency is important, so I'm still open to suggestions on that regard.

Thanks

rxaviers avatar Mar 06 '14 19:03 rxaviers

Renamed project from cldr.js to cldrjs. This way, users can use the same name (cldrjs) on either AMD or CommonJS exports.

rxaviers avatar Apr 08 '14 17:04 rxaviers

The above commit actually didn't help consistency on anything compared to what we had before...

In order to use cldrjs as a module id using require.js, one would need to set config as https://gist.github.com/jrburke/2ddb5ba4adbc2c234248. For more info, see https://github.com/jrburke/requirejs/issues/1084 and https://gist.github.com/rxaviers/10194312.

I think this would bring more complexity for cldr.js users using require.js compared to what we had before:

require.config({
  paths: {
    "cldr": "bower_components/cldrjs/dist/cldr"
  }
});

require( [ "cldr", "cldr/supplemental", "cldr/unresolved" ], function( Cldr ) {
  ...
});

So, I won't make any changes on that regard at this moment.

rxaviers avatar Apr 11 '14 14:04 rxaviers

I'm closing this for lack of activity. Feedbacks are still welcome.

rxaviers avatar Jul 13 '14 04:07 rxaviers

Here's a suggestion:

npm/bower package name: cldrtraverse

JavaScript identifier: cldrTraverse, or CldrTraverse if it refers to a constructor (not required, jQuery is also a constructor), either way, this is userland so it only matters for our usage examples.

AMD: require( [ "cldrtraverse" ], ...

Inspired by names used in messageformat.js

jzaefferer avatar Oct 01 '14 16:10 jzaefferer

@ichernev what do you think about my suggestion in my previous comment?

jzaefferer avatar Dec 11 '14 12:12 jzaefferer

I personally find the concat same case version hard to read. But if that would make it more uniform across platforms its fine.

ichernev avatar Dec 11 '14 18:12 ichernev

I like the idea of making the distinction between both cldrjs (the traverser) and cldr-data more explicit. Maybe cldr-traverse looks better.

rxaviers avatar Dec 11 '14 21:12 rxaviers

I'm okay with cldr-traverse for the package name, following cldr-data instead of messageformat.

jzaefferer avatar Dec 12 '14 15:12 jzaefferer

Alert dependents of this change as possible:

  • [ ] Globalize (code and docs)

rxaviers avatar Dec 19 '14 13:12 rxaviers