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

Node.js support and npm package #20

Closed
3 tasks done
ericf opened this issue Oct 30, 2013 · 31 comments
Closed
3 tasks done

Node.js support and npm package #20

ericf opened this issue Oct 30, 2013 · 31 comments

Comments

@ericf
Copy link
Collaborator

ericf commented Oct 30, 2013

We are interested in using Intl.js in browsers which don't have the built-in Intl APIs and in Node.js.

It's still up in the air on whether Node.js binaries will ship with v8-i18n linked. There's a long discussion with [inaccurate] worries about the size of bundling the required parts of ICU. If this stays a compile-time flag for Node.js, it effectively means that the Intl APIs will not exist in Node.js :(

See: nodejs/node-v0.x-archive#4689 (comment) and nodejs/node-v0.x-archive#6371

I would like to hedge this by being able to use this polyfill as an npm package and run it in Node.js. Would you be on board with adding the umd wrap (or similar) around this code instead of assuming window exists in the runtime? Also would you be on board with publishing this as an npm package?

I am willing to open a PR for this, but I want to make sure that you're on board with this idea before I do so. Thanks!

Todos:

  • Don't assume this is running in a browser with window
  • Add umd wrapper
  • Add package.json and publish to npm
@apipkin
Copy link
Contributor

apipkin commented Oct 30, 2013

👍

1 similar comment
@caridy
Copy link
Collaborator

caridy commented Oct 31, 2013

👍

@ericf
Copy link
Collaborator Author

ericf commented Nov 1, 2013

This is partly covered by #22. I updated the issue with a Todos list.

@caridy
Copy link
Collaborator

caridy commented Nov 26, 2013

hey @andyearnshaw any update on this one?

@andyearnshaw
Copy link
Owner

Nothing to speak of yet. It's a priority for Intl.js (along with completing the flexible building stuff), but I'm just swamped with work at the moment and between that and time spent with my family, I haven't been able to work on personal projects much.

If @ericf still wants to open a PR for it, then I'll make some time to review it, that's absolutely fine with me.

@caridy
Copy link
Collaborator

caridy commented Nov 28, 2013

/ping @drewfish can you take care of this as well since @ericf is pretty busy these days!

@andyearnshaw
Copy link
Owner

Many thanks, @drewfish. Sorry I've been quiet, guys, I've had a lot of personal stuff going on over the past week. Anyhow, I've reviewed and merged but I'm still a little unsure on how I'm going to tackle Intl.js as an NPM module, I think it needs a little more work before publishing.

Right now, Intl.js requires that the locale data is provided by the implementer using the proprietary Intl.__addLocaleData() function. If we publish to NPM as-is, the locale data would be downloaded along with Intl.js, but Intl.js isn't aware of it and will not load it on-demand. This means anyone using Intl.js would still need to separately download the data or look for the node_modules/Intl.js/locale-data folder, which I don't really like.

One option would be to add a small internal function to check if we're running under node and then synchronously load the data instead of throwing the error. Any thoughts?

p.s. thanks for all the interest!

@caridy
Copy link
Collaborator

caridy commented Dec 3, 2013

@andyearnshaw no worries, we will help in any way we can to get all the pieces in place so you just need to review them when you get a chance.

About loading data:

  • loading the data synchronous on the server side is a must. agreed, but loading all the data is probably overkill for most implementations that only need one, or a handful, of locales. In the other hand, we have the issue of having to specify the data on the client side, so doing it on the server side might not be that bit of a hazard in the first place. @drewfish I think we can add an lib/index.js as a CJS file for node to do three things:
    a) load all available lang from that json folder syncronous
    b) call for Intl.__addLocaleData() for each
    c) export Intl
    this way, we don't have to touch Intl.js, and this should be the default behavior whenever you do require('Intl.js');. But if you need/want to control the data that is added thru __addLocaleData(), you should be able to do it manually, like this:
var Intl = require('Intl.js/lib/core');
Intl.__addLocaleData(require('Intl.js/locale-data/json/de-AT.json'));

And that is going to do the exact same, but just adding specific locales instead of all. I will issue a PR to demonstrate this proposal.

@apipkin
Copy link
Contributor

apipkin commented Dec 3, 2013

Would it be possible to wrap it up in a way that the require is hidden?

Intl.__addLocaleData('de-AT');

So under the hood it would do:

if (typeof data === 'string' && __dirname) {
    data = require(__dirname + '/locale-data/json/' + data);
}

@caridy
Copy link
Collaborator

caridy commented Dec 3, 2013

@apipkin I don't like that because:

a) it is an API change in Intl.__addLocaleData(), even though it maintains BC but it does not maintain feature parity with the client side version of the script anymore.
b) the code of the polyfill will now be aware of specific environments (in this case nodejs).

anyway, I issued the PR #26 with the proposal.

@caridy
Copy link
Collaborator

caridy commented Dec 3, 2013

Also, about the question of how to use this thing in nodejs from @andyearnshaw, I think PR #26 describes how is this going to be used. We can add a section dedicated to NodeJS in the README. /cc @drewfish

@apipkin
Copy link
Contributor

apipkin commented Dec 3, 2013

@caridy fair enough :)

@drewfish
Copy link
Contributor

drewfish commented Dec 3, 2013

I dislike the idea that require('Intl.js') pulls in all locales. That seems like a huge amount of data to pull in for little value. There are other ways of making the library easier to use. (I like apipkin's proposal, though it should be documented of course that that shortcut is a) only available on the server, and b) synchronous.)

@drewfish
Copy link
Contributor

drewfish commented Dec 3, 2013

For the client-side (browser) I don't think there's any sugar we can add to make it easier to load locales, since the URL used to load the locale data is very application-specific. So, the only way to make this library behave the same in both environments is to have the user specify the locale data specifically (current behavior).

I like the idea of adding server-specific sugar. There still are people writing node.js applications that don't have much client-side behavior (or, it's not localized).

As well, there are other sources of locale metadata besides this package. (Those sources might need to be massaged to fit into this library.)

@apipkin
Copy link
Contributor

apipkin commented Dec 3, 2013

I think it would be possible for a config to provide a way to have the server side sugar (suggested earlier) become effective on the client side. Again being sychronous and all that. But it could load jsonp data instead and load it later.

There again, if that route were taken, it could be something like:

if (typeof data === 'string') {
    if (__dirname) {
        data = require(__dirname + '/locale-data/json/' + data);
    } else {
        // _load is a generic function whose purpose here is for example only
        // the same goes for config and the callback function
        _load(config.uriToLocaleData + 'data' + .jsonp, Intl.__addLocaleData);
        return;
    }
}

or something along those lines. Again, having it as sugar would lower the front step to getting the system up and running with i18n, but could easily be improved upon by requesting the data in another step and injecting it __addLocaleData as it is currently designed. This would keep it as "load what you want when you want" without having to download everything at one time.

@ericf
Copy link
Collaborator Author

ericf commented Dec 3, 2013

@andyearnshaw using this in the browser requires the person to figure out their own resource bundle loading strategy, I think we should do the same on huge server. The manual loading of resources via require(), similar to what @caridy showed above seems great to me. We can always add the server-side loading sugar later.

Are you worried about the dunder function being part of the public API?

@caridy
Copy link
Collaborator

caridy commented Dec 3, 2013

Ok, ok, let me step back and clarify few things:

a) this is a polyfill, this is not a library, which means we are trying to make up something that is suppose to be there in newer browsers. in other words, when using it in nodejs, you will probably use it like this:

var Intl = global.Intl || require('Intl.js');

that will cover the case where you have nodejs running with the i18n flag (which is coming soon), but also the case where you have an older version of nodejs or the default nodejs configuration. take in consideration that global.Intl is going to have all the locales info available, and you have to do nothing. Which is exactly what the polyfill should be doing.

b) It is true that this is an expensive operation (requiring all files one by one), but it doesn't have to be like that, we can fix that, see point 3.

c) I'm ok inverting the equation and saying that the default Intl.js module will come without langs, but loading the module with all locales data in a single require is a most, so, we could do something like:

var Intl = global.Intl || require('Intl.js/all');

where Intl.js/all is just a generated script as part of the prepublish's npm command which includes the polyfill and adds all locales in a single js file.

d) for the client side, ideally people will use browsify, or some sort of build process to provision Intl.js as a module, and potentially creating other synthetic modules like Intl.js/all or Intl.js/en-US thru a loader implementation and provision them when needed, but that really depends on the implementation of the app, and we should not care much about it from the polyfill perspective.

@apipkin
Copy link
Contributor

apipkin commented Dec 3, 2013

@caridy seeing it from that perspective, i think you raise a good point, but I believe (and correct me if I'm wrong) __addLocaleData() is not part of the original spec, so the inclusion of this already breaks compatability with global.Intl. And with leaning towards spec compatibility, should __addLocaleData() be moved to a supporting lib?

@caridy
Copy link
Collaborator

caridy commented Dec 3, 2013

yes, __addLocaleData is an artifact of the polyfill to provision locales manually. that's precisely something like require('Intl.js/all') will be good enough for the majority of the users. Because the result of that operation give you an object that imitates the standard Intl, at least in some degree.

@andyearnshaw
Copy link
Owner

Wow, busy conversation! I started writing this reply a few hours ago but I didn't get the chance to finish it until now so please disregard anything that's already been said.

I'm in agreement with @drewfish, I wouldn't be happy loading the entire data with require('Intl.js'). I was thinking more along the lines of synchronously loading the locales on-demand and transparently on the server, perhaps even going as far as not exposing __addLocaleData.

I'm toying with the idea of:

  1. Detecting we're running under Node when loaded, and listing files in __dirname + 'locale-data/json/'.
  2. Setting internals.NumberFormat['[[availableLocales]]'] and internals.DateTimeFormat['[[availableLocales]]'] to the result of 1.
  3. Defining a getter for each item in 1 on the [[localeData]] properties that loads the data and then redefines itself to be a static property containing the aforementioned data before returning it.

A potential problem with this approach is choosing a value to be returned by DefaultLocale(), but we could always just go with root.json.

WTR to complete, built-in locale data, I remember starting a solution in the flexibleBuild branch that would have compressed the data by reusing objects where the data is identical (this occurs a lot with number format symbols and datetime formats). IIRC, the approach I took didn't pan out, but I wouldn't mind revisiting it at some point.

@drewfish
Copy link
Contributor

drewfish commented Dec 3, 2013

How do you feel about an optional require('Intl.js/all') to load all intls? In production sometimes it's better to have a larger but known startup cost versus an unknown cost while serving traffic.

@caridy
Copy link
Collaborator

caridy commented Dec 3, 2013

Side note about DefaultLocale(), on the client side this is easy to handle since you have all the information at the browser level, while on the server side, DefaultLocale() should probably never used because the locale detection is, in most cases, bound to the request information, where different requests might trigger different locales.

@andyearnshaw
Copy link
Owner

@drewfish: I'd be OK with that. It's better to give both options as many might prefer the lower footprint of loading data on-demand.

@caridy: yeah, I could see DefaultLocale() being used perhaps during development or testing, but not in production. Originally, I had checking in place for navigator.language || navigator.userLanguage to use as the default locale in browsers, but this didn't really make sense for several reasons (the main being that Intl.js is not guaranteed to have data for that particular locale).

@lwelti
Copy link

lwelti commented Dec 5, 2013

on the server side DefaultLocale() can't be the one from Accept-Language header?

@drewfish
Copy link
Contributor

drewfish commented Dec 5, 2013

@lwelti Yeah I think that's what Caridy meant by "the locale detection is, in most cases, bound to the request information". Since this library has no concept of a connect/express request, it is up to the user of this library to manage the request object to find the locale to use.

@caridy
Copy link
Collaborator

caridy commented Dec 5, 2013

Just to clarify, we cannot use DefaultLocale() on the server side because the same process will be responding to multiple request but Inlt.DefaultLocale() is going to be define at the process level, so you can't have a default locale per request essentially. The solution is to be explicit whenever you're going to use any internationalization feature on the server side in nodejs, while on the client side, you set up the default, and you're not obligated to use the locale token on every call to an intl feature, although, for isomorphic javascript applications we might just use the same code that is always locale aware by passing the explicit locale to be use on every operation.

@andyearnshaw
Copy link
Owner

npm install intl 😄

@caridy
Copy link
Collaborator

caridy commented Dec 10, 2013

image

@ericf
Copy link
Collaborator Author

ericf commented Dec 10, 2013

🤘

@lwelti
Copy link

lwelti commented Dec 10, 2013

great!!

@saran110313
Copy link

features.js compatibility browser support in whiteboard on node.js

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

No branches or pull requests

7 participants