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

[js-joda] add global-exports and prevent overwrite of the top level variable #1904

Closed
wants to merge 2 commits into from

Conversation

henryw374
Copy link
Contributor

No description provided.

@henryw374 henryw374 changed the title add global-exports and prevent overwrite of the top level variable [js-joda] add global-exports and prevent overwrite of the top level variable Jul 2, 2019
@Deraen
Copy link
Member

Deraen commented Jul 2, 2019

I don't understand why the workaround to prevent overwrite of the var is necessary. Is it something special with this library, or a general problem?

@henryw374
Copy link
Contributor Author

henryw374 commented Jul 2, 2019

It's part and part. This library has addon libraries, js-joda-timezone etc, which when loaded, modify the global js-joda object.

It turns out that on node, doing a require of a foreign-lib causes the js code to be re-run. what that means in practice is that in a project with multiple js-joda requires (from different nss), the global js-joda reference is reassigned multiple times and different parts of the project referring to different instances of the global js-joda object. when the addons are loaded, on the most recent is modified.

So, I realise that foreign-libs is not really meant for nodejs, but being that this lib has no dependencies of it's own, I think it would work well enough in this case. IOW - it doesn't hurt to do this and does prevent me getting issues raised by people who just want to quickly try things out on node and don't necessarily realise that there's a foreign lib underneath.

Sorry that turned into a bit of a long answer!

@Deraen
Copy link
Member

Deraen commented Aug 27, 2019

Did you end up solving this in some other way?

I'd probably just recommend documenting that Node users should npm install the library themselves. ClojureScript (and I think Shadow-Cljs) can also automatically install npm packages based on deps.cljs on a library, like here: https://github.com/reagent-project/reagent/blob/master/src/deps.cljs

ClojureScript compiler requires :install-deps true option (and :target :nodejs to emit correct require call based on Cljs namespace require form). I think Shadow-CLJS installs these by default.

@henryw374
Copy link
Contributor Author

henryw374 commented Aug 30, 2019

Hi, thanks for picking this up. I ended up depending on my own version of this lib that includes this change. This cljsjs package is used by my other time libraries and I doubt anyone is using it directly. I realise this change is non-standard but seems like the best compromise for now.

I have now made it clearer to users that npm is preferable, including links to docs on how to exclude the cljsjs dependencies etc: https://github.com/juxt/tick/blob/master/docs/cljs.adoc

Wrt npm-deps, I get the feeling that is on its way to being deprecated, but we'll see.

So, I guess I'll close this now and revert to cljsjs dep if the situation can be resolved in any better way.

@Deraen
Copy link
Member

Deraen commented Aug 30, 2019

I think packaging your own version if perfectly fine if you need some special changes.

@Deraen Deraen closed this Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants