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.

Sign up for GitHub

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

Updating install docs #682

Closed
wants to merge 1 commit into from
Closed

Updating install docs #682

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/install.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ npm install --save luxon
const { DateTime } = require("luxon");
```

If you want to work with locales, you'll need to have `full-icu` support installed in Node. You can [build Node with it](https://github.com/nodejs/node/wiki/Intl), use an [NPM module](https://www.npmjs.com/package/full-icu) to provide it, or find it prepackaged for your platform, like `brew install node --with-full-icu`. If you skip this step, Luxon still works but methods like `setLocale()` will do nothing.
If you want to work with locales, you'll need to have `full-icu` support installed in Node. You can use an [NPM module](https://www.npmjs.com/package/full-icu) to provide it, or find it prepackaged in Node 13+. Please note that trying to install node via brew using `brew install node --with-full-icu` does not work - you're much better off using [nvm](https://github.com/nvm-sh/nvm). If you want to use a Node version prior to 13 _and_ have full icu support, you'll need to build from source using `nvm install <version> -s --with-intl=full-icu --download=all`. If you skip this step, Luxon still works but methods like `setLocale()` will do nothing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've always used the homebrew one. Why doesn't it work?

I'm not loving the explicit recommendation of nvm, since there are tons of tools out there. I think giving the example and keeping agnostic is the best way. Finally, I think it might be better to present this as tree, something like:

  1. If you're using Node 13+, ICU is prepackaged in the default build and no further action is required
  2. If you are using an earlier version of node, you must either:
    A. Install a build of Node with full ICU baked in, such as via nvm: nvm install <version> -s --with-intl=full-icu --download=all or brew: brew install node --with-full-icu
    B. Install the ICU data externally and point Node to it. The instructions on how to do that are below.

If you skip this step, Luxon still works but methods like setLocale() will do nothing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, it just doesn't. I get an error saying that the argument isn't valid.

I can definitely present this a bit differently, making nvm out to be one of a choice (using your language of "such as via x".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also on the nodejs irc channel as part of my debugging this issue, and they recommended I use the nvm versions rather than the Homebrew versions, and it served me well, so I figured the same recommendations should probably apply here.


Please note that the warning above for Node applies when trying to run tests for your web app - those tests will be run using the local default Node version.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't quite understand this clarification. What confusion is this clearing up?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was getting terribly confused before this all started as to why I was getting different results in my tests and in my app, until I remembered that the tests use Node to run, and my app uses webpack. This is more of a piece of text meant to stimulate an 'aha!' moment as people (like my stupid self) might not realise or be too blinkered to step back and realise. I can take it out if you think it isn't necessary, but I think it's useful. Up to you :)


The instructions for using full-icu as a package are a little confusing. Node can't automatically discover that you've installed the it, so you need to tell it where to find the data, like this:

Expand Down