[go: nahoru, domu]
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

Jump to bottom

Simplify Module IDs in Isomorphic Packages #672

Open
mwistrand opened this issue Jan 10, 2017 · 8 comments
Open

Simplify Module IDs in Isomorphic Packages #672

mwistrand opened this issue Jan 10, 2017 · 8 comments
Labels
breaking change

Comments

@mwistrand
Copy link

I have a package that utilizes Globalize.js, and is intended be used in either a CommonJS (Node) or AMD (browser) environment. While it is easy to load everything I need in either environment alone, getting Globalize.js to load correctly in both environments simultaneously is more invovled.

Since the AMD portion of the UMD wrapper references the Cldr.js under the cldr package, additional mappings in the loader configuration are needed to redirect cldr references back to the cldrjs directory so that Node.js doesn't choke when loading the cldr/unresolved that's required in browser environments. In this particular instance, the additional mappings would not be necessary if cldrjs were used as the package ID instead of cldr.

Further, loading the modules underneath globalize/dist/globalize/ with the IDs globalize/message, globalize/date, etc. is easy in an AMD environment with the right loader config. Node, on the other hand, cannot resolve those simplified IDs. As such, the full path is needed to resolve those IDs in both Node and the browser (e.g., require('globalize/dist/globalize/message')). Using the full paths certainly works, but in the long run it would be ideal to use the simplified IDs that do not reference the directory structure.
@mwistrand mwistrand mentioned this issue Jan 10, 2017
Merged
7 tasks
@rxaviers
Copy link
Member

Hi @mwistrand, thanks for filing this issue. I see your pain, thanks for the explanation. You suggested to rename AMD cldr idt to cldrjs. Is there anything else that needs to be done? What's your suggestion please? Thanks

@mwistrand
Copy link
Author

Thanks for responding so quickly, @rxaviers. Yes, changing the Cldr.js module ID from cldr to cldrjs in the AMD require would help reduce the amount of boilerplate required in the AMD loader config. Also, if at all possible, it would be helpful to update to a flat package structure so that module IDs like globalize/message can work in both CommonJS and AMD environments, as opposed to requiring IDs like globalize/dist/globalize/message. That would also be useful in Cldr.js, but that's a different repo and therefore a different issue.

@rxaviers
Copy link
Member
rxaviers commented Jan 16, 2017 •
edited
Loading

Your proposed changes look good to me. One problem I see though is that the cldr to cldrjs name change will be a breaking change for AMD users, right? It will require them to change their loader configs. Can we make it a not breaking change?

module IDS like globalize/message and analogous cldr.js changes.

We should assess the pros and cons of doing that versus releasing individual packages like globalize-message, etc. The benefit is a precise list of dependencies for each of them. Note this second option doesn't necessarily mean we need multiple repos. A release script could take care of releasing multiple packages, e.g., react.js.

@mwistrand
Copy link
Author

I apologize for the delay in responding. Yes, changing cldr to cldrjs would be a breaking change for AMD users, but one that would allow them to simplify their configurations (especially when using an isomorphic third-party package that depends on Globalize.js).

We should assess the pros and cons of doing that versus releasing individual packages like globalize-message, etc.

Personally, I would be fine with relying on individual packages; my main concern is being able to use the same module id in both CommonJs and AMD environments.

@rxaviers
Copy link
Member

@mwistrand would you be interested in submitting a PR with necessary changes here and in cldrjs?

changing cldr to cldrjs would be a breaking change for AMD users, but one that would allow them to simplify their configurations (especially when using an isomorphic third-party package that depends on Globalize.js).

@jzaefferer @scottgonzalez, do you have any suggestion about it please?

@scottgonzalez
Copy link
Contributor

Seems fine for a 2.0.0 release. I'd just say make sure you've dealt with all the issues so that you don't need to do another major release for some other set of module loader problems. Other than that, I don't have much to say; you know how I feel about all these tools...

@rxaviers
Copy link
Member

Thanks for your input @scottgonzalez and yeap I know 😝

@mwistrand
Copy link
Author
mwistrand commented Jan 30, 2017 •
edited
Loading

would you be interested in submitting a PR with necessary changes here and in cldrjs?

Yes, I'm happy to start working on it.

Seems fine for a 2.0.0 release. I'd just say make sure you've dealt with all the issues so that you don't need to do another major release for some other set of module loader problems. Other than that, I don't have much to say; you know how I feel about all these tools...

To be fair, this isn't a question of adding support for an additional tool, but rather one of improving support for a tool that Globalize.js already supports. That said, given what these changes mean for existing users, I had no expectation that they would be included in a minor release.

@rxaviers rxaviers mentioned this issue Mar 9, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking change
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@scottgonzalez @rxaviers @mwistrand