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

clarity on foreign-libs and non-browser js environments #320

Closed
henryw374 opened this issue Jul 3, 2019 · 7 comments
Closed

clarity on foreign-libs and non-browser js environments #320

henryw374 opened this issue Jul 3, 2019 · 7 comments
Labels

Comments

@henryw374
Copy link
Contributor

@henryw374 henryw374 commented Jul 3, 2019

As a result of seeking advice on problems I describe below, I have been advised that foreign-libs are not meant for non-browser environments. If this is the case, I'd like to see that documented on the site. Also, the cljs compiler could output a warning when using foreign-libs, targeting non-browser environments: 'warning: using foreign libs. you should exclude the dependency and provide shim nss as replacements'

If the advice I've got is wrong, no new docs required, but then it does sound like what I see is a possibly a bug in the compiler, or there needs to be more advice on authoring foreign-libs.

Problem description:

In quite a few cases, js scripts inside foreign-lib/cljsjs libraries to have this umd wrapper that (for all cljs envs), runs a function to instantiate a library and assigns the result to a global variable, regardless of the current state. Here's an example of what you get with cljsjs/react: https://unpkg.com/react-dom@16.7.0/umd/react-dom.development.js

When running cljs on node, (e.g. clj -m cljs.main -re node -m myscript), the foreign lib script is re-run every time you do a foreign lib require, e.g. (require 'cljsjs/react). In the case of cljsjs/react, this means the global js/React variable is reassigned to the result of running the script. This is unlike the behaviour that require has normally unless you do a (require ... :reload) and is different to what you get when running cljs in a browser.

@mfikes
Copy link
Member

@mfikes mfikes commented Jul 3, 2019

I've never heard this advice.

@henryw374
Copy link
Contributor Author

@henryw374 henryw374 commented Jul 3, 2019

Thanks Mike - to clarify, this was advice from Thomas Heller, a well-respected voice in the cljs community I think it's fair to say.

from slack cljs channel:
"foreign-libs are not really meant for node environments so I'd expect that to have all sorts of issues and especially cljsjs is targeted at the browser"

If foreign-libs should work in node, then it looks like the require behaviour I've seen is a bug. Would you agree?

@mfikes
Copy link
Member

@mfikes mfikes commented Jul 3, 2019

But at its core, :foreign-libs is really about just pulling in some JavaScript source. It is the simplest (most naïve) way of depending on some source by blindly prepending it to your output.

Perhaps that's the problem in that this approach might fail if the JavaScript you are prepending has special semantics in certain environments?

There is probably something more subtle to do here than a blanket statement saying that :foreign-libs is not specifically for Node-based targets. (Because, as a counter argument, I could write some plain JavaScript, want to depend on it, and it should be fine pulling that in via :foreign-libs into my project.)

Maybe this is better first sorted out via a JIRA, presuming there is a bug that can be solved?

@henryw374
Copy link
Contributor Author

@henryw374 henryw374 commented Jul 3, 2019

Thanks, I'll leave this open for a bit to see if anyone else want to weigh in?

FWIW my workaround for this problem is to add code to the foreign-lib to check if the resulting global variable has already been assigned (cljsjs/packages#1904). But obviously this is been done as a one-off. So if there is no bug to fix, then perhaps what's missing is some advice to say something like 'foreign libs should expect to be run any number of times so as author you need to manage any problems that arise from that'

@thheller
Copy link
Contributor

@thheller thheller commented Jul 5, 2019

@mfikes the problem is indeed in how most of the cljsjs packages are packaged. Many of them are either wrapped in a typical UMD wrapper or are in fact produced by a webpack build which will in turn include certain browser specific aspects. :foreign-libs themselves can work fine in node, but most of them don't.

As @henryw374 pointed out the most common issue will be the UMD wrapper. So lets break that down based on the linked example of react-dom. Although react-dom is not really that relevant to node but the pattrern is common and this is an easy example.

(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(require('react')) :
	typeof define === 'function' && define.amd ? define(['react'], factory) :
	(global.ReactDOM = factory(global.React));
}(this, (function (React) { 'use strict';

This wrapper will try to pull in its react dependecy by whatever mechanism is available in the given runtime. In the Browser this will resort to using the global React variable (which cljsjs/react) provided. However in node the first test will succeed so it will load its react dependency directly from the node package. Since the cljsjs packaged variant was also loaded when it was prepended we now have two conflicting versions of react loaded. CLJS code will refer to the global React variable but react-dom will not.

Hence my statemenent that CLJSJS in general is targetted for the Browser and may cause issues elsewhere.

If configured correctly the cljsjs packages shouldn't be included at all in node builds and instead the react require should be mapped to an actual require("react"). The compiler is capable of this but I'm not sure of all the details regarding this since shadow-cljs handles this differently.

@henryw374
Copy link
Contributor Author

@henryw374 henryw374 commented Jul 5, 2019

Ok, thanks everyone.

To sum up then:

  • foreign-libs are intended for any cljs env, but typical cljsjs/umd ones may have issues when in a non-browser env.
  • It isn't a bug that on node, a foreign-lib's script is run on every call to require.
  • cljs libraries depending on node libs shouldn't do so via foreign-libs

I created this demo https://github.com/henryw374/cljs-node-foreign-lib-demo for posterity.

It feels to me like what I've learned could be documented better somewhere, possibly on this site. Maybe in the future ie when everyone is using: https://gist.github.com/swannodette/ebd8b65f887318ba68579b6bea911daf, then the kind of confusion I've had will cease to be an issue.

@swannodette
Copy link
Member

@swannodette swannodette commented Apr 12, 2020

Closing this. :target :bundle is around the corner and this is going to become something only experts need care about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants