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

export name #7

Open
ichernev opened this issue Mar 2, 2014 · 14 comments
Open

export name #7

ichernev opened this issue Mar 2, 2014 · 14 comments

Comments

@ichernev
Copy link

ichernev commented Mar 2, 2014

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?

@rxaviers
Copy link
Owner

rxaviers commented Mar 6, 2014

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.

@ichernev
Copy link
Author

ichernev commented Mar 6, 2014

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 :)

@rxaviers
Copy link
Owner

rxaviers commented Mar 6, 2014

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.

@ichernev
Copy link
Author

ichernev commented Mar 6, 2014

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.

@rxaviers
Copy link
Owner

rxaviers commented Mar 6, 2014

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
Copy link
Owner

rxaviers commented Apr 8, 2014

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

@rxaviers rxaviers reopened this Apr 11, 2014
@rxaviers
Copy link
Owner

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 requirejs/requirejs#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
Copy link
Owner

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

@jzaefferer
Copy link

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

@rxaviers rxaviers reopened this Oct 1, 2014
@jzaefferer
Copy link

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

@ichernev
Copy link
Author

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

@rxaviers
Copy link
Owner

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

@jzaefferer
Copy link

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

@rxaviers
Copy link
Owner

Alert dependents of this change as possible:

  • Globalize (code and docs)

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

No branches or pull requests

3 participants