rxaviers / cldr-data-npm

Npm module for Unicode CLDR JSON data
MIT License
42 stars 27 forks source link

How to get full coverage in a global npm module #6

Closed tinganho closed 9 years ago

tinganho commented 9 years ago

HI @rxaviers

I wonder how I can get the full coverage in L10ns, without telling my users to set the global variable CLDR_COVERAGE?.

It would be good if i can "npm install l10ns -g" and it will install cldr-data with full coverage.

rxaviers commented 9 years ago

Hello @tinganho,

I'm assuming you have read https://github.com/rxaviers/cldr-data-npm/issues/2.

It's not clear to me if you: (a) don't want to give your users the choice to choose the locale coverage, or (b) you want the full coverage to be the default.

tinganho commented 9 years ago

Missed the issue #2 , but seems very related to my issue.

I don't want to give them a choice of choosing. I just want the full coverage. Because I think it easier for them.

rxaviers commented 9 years ago

I'm thinking... One of the biggest challenges cldr-data had was specifically to give users the freedom to choose coverage (while libraries choose functionality).

Our original use case was to allow libraries to define CLDR data as versioned peer dependency. It seems to me like your use case is a little different. I'm understanding l10ns is similar to an application, where you want CLDR data as a real dependency, since you want users to install l10ns globally (npm install l10ns -g). Therefore, you want to choose the coverage yourself and package all that.

Do you think coming up with a custom entry on package.json to specify the coverage is a good idea? E.g.,

{
    "dependencies": {
        "cldr-data": "26"
    },
    "cldr-data-coverage": "full"
}
rxaviers commented 9 years ago

cc @jzaefferer

jzaefferer commented 9 years ago

Extending package.json with a property that this module reads, as an alternative to ENV variables, sounds good to me (I think we indeed discussed that before).

tinganho commented 9 years ago

@rxaviers yes, I like the config idea in package.json.

rxaviers commented 9 years ago

We just need to make sure this property is only taken into account when "cldr-data" is defined in "dependencies", not "peerDependencies".

tinganho commented 9 years ago

@rxaviers any plan on when to make this happen? I have now changed out everything on l10ns to use cldd-data together with cldrjs. But I still rely on the environmental variable.

tinganho commented 9 years ago

@rxaviers I just found out that there is a dedicated config setting in package.json. It's maybe worth to take a look. https://docs.npmjs.com/files/package.json#config. Maybe you can add CLDR version and coverage there.

rxaviers commented 9 years ago

Where's this config property defined? In the package.json of this module or in the package.json of modules that use it?

tinganho commented 9 years ago

I think they mean of package.json for modules that uses it. Since they are mentioning npm start.

Though, my point is that might be cleaner to set the config there than to set it in a custom config property cldr-data-coverage?

rxaviers commented 9 years ago

I'm asking because using our custom cldr-data-coverage, we're able to check whether this has been set in a end-user package.json. Are we able to say the same for the property set in config? Given modules that use it can set a value, what happens if two modules try to override? Which "wins"?

I'm very interested in using this property instead. It seems a correct path. But, I'm still a little confused about how it works.

tinganho commented 9 years ago

I'm asking because using our custom cldr-data-coverage, we're able to check whether this has been set in a end-user package.json. Are we able to say the same for the property set in config

Yes, at least if we do the same check by checking the parent package.

Given modules that use it can set a value, what happens if two modules try to override? Which "wins"?

I don't think the config is meant to be overridden. They are just meant to be read.

I just tested it. It seems that it should be used together with npm start. It sets an env var npm_config_NAME. Then, the end-user can override it by npm config set key value. So this config property is not 100% dedicated for cldr-data's configs. Seems like the npm config property is more of a runtime config and what cldr-data need is a install config.

jzaefferer commented 9 years ago

npm encourages adding custom fields to package.json. I still think that's the right approach here.

PS: Reopen this issue?

tinganho commented 9 years ago

@jzaefferer I think I agree on that the custom field is a better approach after doing some research.

rxaviers commented 9 years ago

What we currently have is a custom field. Is there any change needed here still?

+55 (16) 98138-1582, +1 (415) 568-5854, skype: rxaviers http://rafael.xavier.blog.br

tinganho commented 9 years ago

@rxaviers I don't think so.

dpolivy commented 4 years ago

@rxaviers Let me know if I should open a new issue for this, but the way the code is implemented to use the custom property is making an assumption about the node_modules folder arrangement that is not even guaranteed by classic NPM. It also means it is not so compatible with newer package managers like PNPM and Yarn Plug'n'Play. I think it's generally not a good recommendation to hardcode relative paths like this. (And it also means that I'm not able to get the full data download when installing with pnpm, at least using the package.json property.)