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

Intl #238

Closed
rvagg opened this issue Jan 6, 2015 · 42 comments
Closed

Intl #238

rvagg opened this issue Jan 6, 2015 · 42 comments
Labels
build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation.

Comments

@rvagg
Copy link
Member

rvagg commented Jan 6, 2015

This has landed in joyent/node from what I understand and from nodejs/node-v0.x-archive#7676 (comment) it seems that we may be adjusting builds to do ./configure --with-intl=small-icu and vcbuild.bat small-icu if we are to stay in step.

As far as I know @piscisaureus has been the only one to raise objections to the way Intl is being handled, specifically with regard to the download step and increase in binary size even with small-icu. Is there anything we need to discuss here or is that a discussion for joyent/node and we're just going to follow along?

Then there's our impending 1.0.0-alpha release. I'd really like to see the Intl changes land in io.js ASAP so we can get them properly tested in our CI and be part of our initial release. Then we just have to agree about the build configuration for release.

  • Action: someone land the Intl changes here please
  • Action: discussion required about how io.js handles Intl, whether small-icu is enabled for releases and what we enable for general CI builds
@bnoordhuis
Copy link
Member

/cc @srl295

@bnoordhuis
Copy link
Member

By the way:

Then there's our impending 1.0.0-alpha release. I'd really like to see the Intl changes land in io.js ASAP so we can get them properly tested in our CI and be part of our initial release.

This is the case after yesterday's merge in commit 94e1475.

@rvagg rvagg added this to the v1.0.0 milestone Jan 12, 2015
@rvagg
Copy link
Member Author

rvagg commented Jan 12, 2015

put on the v1.0.0 milestone as this needs a final decision, we didn't get much TC engagement in the last meeting on this and I want to be sure we want to go live with 1.0.0 without even small-icu

@rvagg rvagg mentioned this issue Jan 12, 2015
14 tasks
@bnoordhuis
Copy link
Member

I want to be sure we want to go live with 1.0.0 without even small-icu

+1 from me. Intl is a nice-to-have at this point, not a blocker.

@rvagg
Copy link
Member Author

rvagg commented Jan 13, 2015

going to cross this off the list for the purposes of 1.0.0 now, confirmation from @bnoordhuis and @piscisaureus is enough for me (particularly given that they are the ESL members of the TC)

@ruimarinho
Copy link

Just my two cents - we should postpone this and discuss how we want to integrate with Intl more thoroughly. Shipping with small-icu which, afaik, is equivalent to ship the full API but with just English support, doesn't bring anything special to the table to 1.0.0. @rvagg shouldn't this be re-added to the next tc-agenda (after 1.0.0)?

@rvagg
Copy link
Member Author

rvagg commented Jan 13, 2015

@ruimarinho perhaps, if you feel it important enough then you could remind us prior to a TC meeting, I'm not going to label it right now

ruimarinho referenced this issue in smockle-archive/homebrew-iojs Jan 13, 2015
@Fishrock123 Fishrock123 removed this from the v1.0.0 milestone Jan 29, 2015
@keithamus
Copy link

I have no idea if this will be helpful to anyone (seems like maintains are very much aware of this), but I'm currently working on a PR for testing Intl support on Kangax's Compat Table.

Here's the ouput:
screen shot 2015-02-12 at 21 39 44

@rvagg
Copy link
Member Author

rvagg commented Feb 13, 2015

We have a standing item to try and get @srl295 on to a TC meeting to discuss this in more depth, I'll keep chipping away at him and we may get him to agree some time! The main concerns are about packing size and @piscisaureus has been doing some work on that too.

@srl295
Copy link
Member

srl295 commented Feb 13, 2015

@rvagg yes and - as to size - faq entry and my nodesummit presentation discusses some. .. i'm working on some other PRs around the data packaging and docs also.

@srl295
Copy link
Member

srl295 commented Feb 19, 2015

@rvagg hi, was any of the above related to your questions?

@rvagg
Copy link
Member Author

rvagg commented Feb 19, 2015

wasn't so much me that had questions, the TC is likely to remain a -0 on bundling Intl support for the moment because of concerns about size

@srl295
Copy link
Member

srl295 commented Feb 20, 2015 via email

@Fishrock123
Copy link
Member

ping @piscisaureus what was the status of your Intl/ICU work?

@stuartpb
Copy link

Here's my non-expert opinion on how Intl should look in core io.js / #978 Node (apologies if this is infeasible or obvious):

  • Have the functions be present as part of core, but with limited functionality out-of-the-box (shipping with one locale/small-icu as the default build).
  • Have the core Intl functionality look for a specific official module or modules, to be installed by npm, to load other locales.

So, for example, if I wanted full international support in my project, I could just do something like npm install --save full-icu, and then when running node app.js stuff like new Date().toLocaleString('pt-BR') would Just Work.

@piscisaureus
Copy link
Contributor

@Fishrock123
Although it seems theoretically possible to make ICU much smaller, I don't have time to do it. And I kind of lack the motivation to become an ICU expert.

I totally agree with @stuartpb's suggestion. I think this would require some changes to libicu though, but it's not nearly as complicated as the thing I was trying to do.

@jasnell
Copy link
Member

jasnell commented Mar 20, 2015

This is an area that @srl295 and I can explore further (although, admittedly it would likely be more @srl295 since he's the expert ;-) ...)

@jbergstroem
Copy link
Member

@jasnell Have you perhaps had any insights with regards to this?

Being able to do LD_PRELOAD=libicu.so iojs (or as @stuartpb suggested, convenience wrappers through npm) feels like the perfect compromise to me.

@srl295
Copy link
Member

srl295 commented Apr 3, 2015

@stuartpb :

apologies if this is infeasable

It's all made of code, right? :) However, the Intl integration is at the v8 level, so some things are more difficult at this point.

@jbergstroem :

LD_PRELOAD=libicu.so iojs

Yes, but again that means removing the support from the runtime layer of v8 and rewriting it as a shared library that registers (?) itself into v8.. I did not think v8 had that kind of module support.

@stuartpb : Edit Must be too early. I just about rewrote this whole comment. Basically, yes. As the numbers in the faq "core" support adds 5MB to the binary size. nodejs/node-v0.x-archive#8979 is about reducing that a little more, even. I've just put the data to add additional support in an npm module. The main issue is auto-discovery of this data- I guess at startup, looking in the global node_modules for the data package? If it's there, use it?

@stuartpb
Copy link

stuartpb commented Apr 3, 2015

The main issue is auto-discovery of this data- I guess at startup, looking in the global node_modules for the data package? If it's there, use it?

That's what I was thinking (either at startup or at the first call of an Intl function).

@srl295
Copy link
Member

srl295 commented Apr 3, 2015

@stuartpb cool, i will take a look at this option.

@ChALkeR
Copy link
Member

ChALkeR commented May 18, 2015

Does this mean that this will be fixed by merging changes from node?

@bnoordhuis
Copy link
Member

Yes. I'll close the issue. Once it's merged, we can look at making it better.

@srl295
Copy link
Member

srl295 commented Jul 15, 2015

@silverwind normalization is needed in lots of contexts.

@silverwind
Copy link
Contributor

@srl295 yeah, saw a issue about it. Personally I'm in definately favor of the Intl utility methods, many of these are very useful. I ran a small-icu build which bumped the binary from 16 to 21 MB, which looks acceptable.

One small concern would be the perf impact of doing normalization on every string returned by fs for example. Are these methods heavy?

@srl295
Copy link
Member

srl295 commented Jul 15, 2015

@silverwind they are designed to be fast. I don't know the speed off hand. Is there a simple way to measure it in Node?

@silverwind
Copy link
Contributor

@srl295 there are the benchmark scripts which could probably give you an idea, but I'm not particulary familiar with them either yet.

As a fix for the OS X issue I think we could just limit the normalization to Darwin >= 15.0.0, as all other platforms normalize automatically as of today. If we ever notice other platforms doing the same, we could then enable normalization on them individually. That way, there should not be much an impact on unaffected platforms.

@srl295
Copy link
Member

srl295 commented Jul 15, 2015

@silverwind Let's discuss that over on #2165

@silverwind
Copy link
Contributor

Anyone got a link to the change that enabled Intl in 0.12 builds? I can't seem to find any recent changes in the Makefile over at joyent/node.

@jasnell
Copy link
Member

jasnell commented Jul 19, 2015

@srl295 can point you in the right direction
On Jul 18, 2015 5:40 PM, "silverwind" notifications@github.com wrote:

Anyone got a link to the change that enabled Intl in 0.12 builds? I can't
seem to find any recent changes in the Makefile over at joyent/node.


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

@silverwind
Copy link
Contributor

Nevermind, found it: nodejs/node-v0.x-archive@67f87a7. @srl295 would you mind porting it to io.js's master? If there are any objections to it (which I doubt), we can discuss those in the PR.

@jasnell
Copy link
Member

jasnell commented Jul 19, 2015

@silverwind : yeah, this was discussed on the last tsc call. @srl295 and I have the action to get that PR put together. Realistically it'll be early to mid next week.

@silverwind
Copy link
Contributor

@jasnell great!

FYI, #2165 doesn't strictly depend on it if we delegate unicode normalization to the user, but it'd be nice to point users to String.prototype.normalize as an advice.

@Fishrock123 Fishrock123 added the build Issues and PRs related to build files or the CI. label Jul 20, 2015
@srl295
Copy link
Member

srl295 commented Jul 22, 2015

porting to io.js's master

@silverwind yes my issue for doing that is #26 and I"m working on doing exactly that

@silverwind silverwind added the i18n-api Issues and PRs related to the i18n implementation. label Aug 10, 2015
@srl295
Copy link
Member

srl295 commented Aug 11, 2015

As I understand it, this specific issue should be closed when #2264 lands. Please let me know if anyone understands differently.

srl295 added a commit that referenced this issue Aug 13, 2015
* turn on small-icu by default for builds (Makefile+Windows)
* add license info from ICU
  http://source.icu-project.org/repos/icu/icu/trunk/license.html
  All text pasted. Long lines wrapped. (original is HTML.)

Port from joyent/node of:
* #26
 * port of joyent/node 67f87a7
* nodejs/node-v0.x-archive#9038
 * Merge from joyent/node 70d04e7
 * Merge from joyent/node 6168fe6
 * merge from joyent/node e670732

PR-URL: #2264
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Fixes: #238
@srl295
Copy link
Member

srl295 commented Aug 13, 2015

Landed in 2781333

@srl295 srl295 closed this as completed Aug 13, 2015
srl295 added a commit that referenced this issue Aug 17, 2015
* turn on small-icu by default for builds (Makefile+Windows)
* add license info from ICU
  http://source.icu-project.org/repos/icu/icu/trunk/license.html
  All text pasted. Long lines wrapped. (original is HTML.)

Port from joyent/node of:
* #26
 * port of joyent/node 67f87a7
* nodejs/node-v0.x-archive#9038
 * Merge from joyent/node 70d04e7
 * Merge from joyent/node 6168fe6
 * merge from joyent/node e670732

PR-URL: #2264
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Fixes: #238
@srl295
Copy link
Member

srl295 commented Dec 16, 2015

Postscript: ICU upstream is going to adopt a plaintext license as formatted for node. http://bugs.icu-project.org/trac/ticket/12037

@jasnell
Copy link
Member

jasnell commented Dec 16, 2015

@srl295 +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

No branches or pull requests