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

[Proposal] Deprecate --without-intl compilation flag #35942

Open
aduh95 opened this issue Nov 3, 2020 · 21 comments
Open

[Proposal] Deprecate --without-intl compilation flag #35942

aduh95 opened this issue Nov 3, 2020 · 21 comments
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@aduh95
Copy link
Contributor

aduh95 commented Nov 3, 2020

Is your feature request related to a problem? Please describe.

Currently, Node.js supports the compilation with disabled ICU. I think we would benefit from officially deprecating it:

Describe the solution you'd like

Having a warning in the console when compiling using --without-intl flag, and document that Node.js versions compiled without ICU are expected to have some unavailable features.

Also, I think the project should allow initiatives like #27662 to land even if it doesn't work on --without-intl node versions.

If you are yourself a user of non-ICU versions of Node.js, please join the discussion, I'd be interested to hear what are the use-cases.

cc/ @nodejs/tsc

@mcollina
Copy link
Member

mcollina commented Nov 3, 2020

+1

@targos
Copy link
Member

targos commented Nov 3, 2020

Do we know if anyone builds Node with that option in the wild?

@mmomtchev
Copy link
Contributor

Users of nexe, pkg and boxednode (@addaleax)

@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2020

I use it for reducing the binary size.

@addaleax addaleax added the i18n-api Issues and PRs related to the i18n implementation. label Nov 3, 2020
@addaleax
Copy link
Member

addaleax commented Nov 3, 2020

Yeah, I think it’s still a decent option for decreasing binary size if the target environment profits from this. To be clear, I’m not using this myself – just saying I can see that there are valid use cases.

@targos
Copy link
Member

targos commented Nov 3, 2020

What is the difference in size between --without-intl and "small ICU" ?

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 3, 2020

To be clear, I'm not advocating for removal the compilation option. Maybe deprecating is not the correct wording, but I think we need to rethink the status of those builds.
I've added to the OP an example of a PR (#27662) currently blocked because of ICU which may be relevant to the discussion.

@astefanutti
Copy link

@targos

What is the difference in size between --without-intl and "small ICU" ?

You can find the size differences here:

astefanutti/scratch-node@8a588ed#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5L15-L19

After I switched from --without-intl to --with-intl=small-icu in https://github.com/astefanutti/scratch-node.

@johan13
Copy link

johan13 commented Nov 7, 2020

I use it.
I run node.js on embedded devices that are updated OTA and we pay for every MB downloaded. I would like to keep --without-intl.

@branchseer
Copy link
Contributor

I use it for https://github.com/patr0nus/rust-nodejs.
It's a cargo library that embeds Node.js in Rust. It provides a no-intl feature which links Node.js compiled with --without-intl so the library consumer can get a smaller binary size.
See https://github.com/patr0nus/libnode/releases for the size differences of Node.js compiled as static libraries.

@Trott
Copy link
Member

Trott commented Dec 4, 2021

I think we can/should leave --without-intl as an option for people who need it, but I wonder if we could/should reduce the amount of testing we do on it. It's already basically a "might break stuff unexpectedly" option. (Maybe we should make that more clear in the doc, if it's not already sufficiently clear.)

My life would have been complete without taking time to figure out how to keep Unicode syntax highlighting from breaking the documentation build on intl. We landed something, @richardlau had to roll it back (because no one thought "this needs to be tested on full CI", which I guess is a separate issue to address), and then we had a discussion (involving @targos and @aduh95 too) about how to get it working.

I guess we could skip building docs if building --without-intl, but we may also need to skip various doc tests too.

@ChALkeR
Copy link
Member

ChALkeR commented Dec 4, 2021

E.g. #35092 (review) is an example of what could have been broken with --without-intl.

@Trott
Copy link
Member

Trott commented Dec 4, 2021

E.g. #35092 (review) is an example of what could have been broken with --without-intl.

@ChALkeR Do you mean supporting --without-intl is holding back things like that/making them more complicated? Or do you mean that it's important to support --without-intl to prevent certain mistakes from happening?

@ChALkeR
Copy link
Member

ChALkeR commented Dec 4, 2021

I meant only that there are PRs that are seemingly unrelated to intl on the first glance, that might get broken on --without-intl if we don't test for that on CI.

@Trott
Copy link
Member

Trott commented Dec 4, 2021

I meant only that there are PRs that are seemingly unrelated to intl on the first glance, that might get broken on --without-intl if we don't test for that on CI.

Yeah, I almost want to suggest that our CI for --without-intl involve making sure building succeeds and then maybe doing some absolutely minimal "hello world"-style tests. In other words, we'll try not to break the build, and we'll try to make sure the thing runs, but we won't guarantee that there won't be functionality issues, some of which might be surprising.

@mscdex
Copy link
Contributor

mscdex commented Dec 5, 2021

Yeah, I almost want to suggest that our CI for --without-intl involve making sure building succeeds and then maybe doing some absolutely minimal "hello world"-style tests. In other words, we'll try not to break the build, and we'll try to make sure the thing runs, but we won't guarantee that there won't be functionality issues, some of which might be surprising.

Do you mean something beyond what the ubuntu1804_sharedlibs_withoutintl_x64 worker does (which is used by node-test-commit-linux-containered)?

@Trott
Copy link
Member

Trott commented Dec 5, 2021

Yeah, I almost want to suggest that our CI for --without-intl involve making sure building succeeds and then maybe doing some absolutely minimal "hello world"-style tests. In other words, we'll try not to break the build, and we'll try to make sure the thing runs, but we won't guarantee that there won't be functionality issues, some of which might be surprising.

Do you mean something beyond what the ubuntu1804_sharedlibs_withoutintl_x64 worker does (which is used by node-test-commit-linux-containered)?

I mean less than that because that runs the full test suite.

@mscdex
Copy link
Contributor

mscdex commented Dec 5, 2021

I think we should be going the other direction on that -- making sure more PRs go through a full CI run. A full CI run doesn't even take that long, considering there isn't anything that really needs to be merged before the time it takes for a full CI run to complete.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 5, 2021

I disagree, I don't think more CI runs is an appropriate response:

  • no matter how many CIs we spawn, there will always be a non-zero chance of something to break;
  • running a full CI does take that long, especially due to the flakiness of some Jenkins hosts;
  • the number of times we had to fast track a PR to fix the CI is quite small that the current state of things is ok.

The most recent breakage of the CI came from highlight.js using a regex pattern unsupported on withoutintl build.. if the ecosystem doesn't support withoutintl, we will have a hard time doing so as we rely on other packages to build the docs. imho there's no reason for us to ensure the docs can be built on a withoutintl build, we can decide it requires at least small-ICU. So I agree with @Trott, testing less would be the correct response.

@mscdex
Copy link
Contributor

mscdex commented Dec 5, 2021

there's no reason for us to ensure the docs can be built on a withoutintl build

That's fine as long as CI either doesn't try to build docs before running any actual tests or skips building docs if intl isn't available so that tests can still run.

@Trott
Copy link
Member

Trott commented Dec 5, 2021

skips building docs if intl isn't available

That's the path in #41091.

One issue we have is that we don't run --without-intl builds on Windows. So I couldn't take the Makefile changes and make equivalent changes to vcbuild.bat with confidence.

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

No branches or pull requests