ClojureScript

Support ns :require for JS libs, allow strings along with symbols

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: 1.9.562
  • Fix Version/s: 1.9.854
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Accepted

Description

Requiring JS code from CLJS requires a lot of manual JS interop and can be simplified by allowing Strings in the ns form.

Instead of using (js/require "react") or relying on a provided global js/React it should be possible to write

(ns demo.foo
  (:require ["react" :as react :refer (createElement)])
  (:import ["react-native" Text View]))

It is somewhat at odds with the current :foreign-libs and CLJSJS ecosystem since the intent is to directly consume NPM packages instead of relying on re-packaged NPM packages. node.js is able to directly use these requires but other runtimes (eg. browsers) require an additional build step that provides the NPM packages. :foreign-libs could probably be enhanced to provide "react" instead of cljsjs.react and then js/React.

I don't know what it would take to bring this into CLJS but I think it would be a useful feature. I have a working implementation in the shadow-cljs project [1] but unfortunately that is not easily portable due to significant differences in the compilation process. It currently only works for requiring packages (ie. "react") but not for relative requires (ie. "./foo") but :foreign-libs has the same limitation.

[1] https://github.com/thheller/shadow-cljs

Activity

Hide
Thomas Heller added a comment -

Some updates on the subject so they don't get lost in slack.

Assuming that the string requires just map to require JS it all just works in node which is very convenient.

For the browser however some manual work needs to be done in order for them to work.

If the build just produces one output file you can run that through webpack and it will fill in the require parts but it requires running through webpack. It also doesn't work properly in :none since webpack doesn't follow the goog.require("cljs.core"); statements. shadow-cljs has an option to output CommonJS compatible code to make this work but this is not available in core.

One issue that required working around were relative requires. Since the JS runtime would assume them to be relative to the generated JS file which lives in :output-dir but the require should be relative to the source file which requires rewriting by the compiler. Since manual (js/require ...) is just code we cannot easily rewrite that. The ns require is just data that can easily be rewritten.

:npm-deps or :foreign-libs could definitely help making things easier for the browser.

They should however continue to use js/require and just provide a shim if the user doesn't want to use the native implementation. webpack or node should never rely :foreign-libs as they would cause conflict.

Show
Thomas Heller added a comment - Some updates on the subject so they don't get lost in slack. Assuming that the string requires just map to require JS it all just works in node which is very convenient. For the browser however some manual work needs to be done in order for them to work. If the build just produces one output file you can run that through webpack and it will fill in the require parts but it requires running through webpack. It also doesn't work properly in :none since webpack doesn't follow the goog.require("cljs.core"); statements. shadow-cljs has an option to output CommonJS compatible code to make this work but this is not available in core. One issue that required working around were relative requires. Since the JS runtime would assume them to be relative to the generated JS file which lives in :output-dir but the require should be relative to the source file which requires rewriting by the compiler. Since manual (js/require ...) is just code we cannot easily rewrite that. The ns require is just data that can easily be rewritten. :npm-deps or :foreign-libs could definitely help making things easier for the browser. They should however continue to use js/require and just provide a shim if the user doesn't want to use the native implementation. webpack or node should never rely :foreign-libs as they would cause conflict.
Hide
Juho Teperi added a comment -

As a test I converted Reagent & Reagent demo site to use npm React: https://github.com/reagent-project/reagent/compare/npm-deps

I had to use foreign-libs list instead of npm-deps to support be able to use react-dom/server. I think this is currently the biggest problem with npm package support. As workaround I ran node-inputs with manually written JS file with 4 require calls: React, React-dom, React-dom/server and create-react-class. I also added :produces entry for react-dom/server foreign lib.

If I understand npm-deps correctly, the value of the option is used to create a JS file with require calls to the package index files. This file is used to index the dependency tree of the packages with node-inputs. Problem with this is that the dependency tree won't include for example react-dom/server.js as that is not a dependency of any of index files. This means that even if there was a require to react-dom/server, that file can't be found.

  • We need a way to present require to react-dom/server, could be the string requires or just a way to map this to react-dom.server
  • Npm index needs to include required files
Show
Juho Teperi added a comment - As a test I converted Reagent & Reagent demo site to use npm React: https://github.com/reagent-project/reagent/compare/npm-deps I had to use foreign-libs list instead of npm-deps to support be able to use react-dom/server. I think this is currently the biggest problem with npm package support. As workaround I ran node-inputs with manually written JS file with 4 require calls: React, React-dom, React-dom/server and create-react-class. I also added :produces entry for react-dom/server foreign lib. If I understand npm-deps correctly, the value of the option is used to create a JS file with require calls to the package index files. This file is used to index the dependency tree of the packages with node-inputs. Problem with this is that the dependency tree won't include for example react-dom/server.js as that is not a dependency of any of index files. This means that even if there was a require to react-dom/server, that file can't be found.
  • We need a way to present require to react-dom/server, could be the string requires or just a way to map this to react-dom.server
  • Npm index needs to include required files
Hide
Thomas Heller added a comment -

The point of this is to refer to npm packages by their actual name, never by a made up alias (ie. symbol). The user always uses strings and never the internal alias.

shadow-cljs currently does this:

;; orig
(:require ["react-dom/server" :refer (render)])
;; rewritten to
(:require [shadow.npm.react_dom_SLASH_server :refer (render)])
;; shadow.npm.react_dom_SLASH_server could be foo$ADFASDFASDFAKSDJFASDKFAJDS as it doesn't matter in any way.
;; pseudo ns
goog.provide("shadow.npm.react_dom_SLASH_server");
shadow.npm.react_dom_SLASH_server = require("react-dom/server");

I do it this way because I specifically do not want things to run through the Closure Compiler (and it didn't require any changes to CLJS).

:npm-deps could map "react-dom/server" to the name generated by the Closure Compiler. It does not need to emit require or the pseudo ns since there will be actual namespaces behind it.

Since the ns form can be statically analyzed you can also just query the analyzer state for the required npm packages and use that as the entries for node-inputs.

:foreign-libs remain useful for externs and maybe could be used to provide pre-bundled JS packages for the browser.

Show
Thomas Heller added a comment - The point of this is to refer to npm packages by their actual name, never by a made up alias (ie. symbol). The user always uses strings and never the internal alias. shadow-cljs currently does this:
;; orig
(:require ["react-dom/server" :refer (render)])
;; rewritten to
(:require [shadow.npm.react_dom_SLASH_server :refer (render)])
;; shadow.npm.react_dom_SLASH_server could be foo$ADFASDFASDFAKSDJFASDKFAJDS as it doesn't matter in any way.
;; pseudo ns
goog.provide("shadow.npm.react_dom_SLASH_server");
shadow.npm.react_dom_SLASH_server = require("react-dom/server");
I do it this way because I specifically do not want things to run through the Closure Compiler (and it didn't require any changes to CLJS). :npm-deps could map "react-dom/server" to the name generated by the Closure Compiler. It does not need to emit require or the pseudo ns since there will be actual namespaces behind it. Since the ns form can be statically analyzed you can also just query the analyzer state for the required npm packages and use that as the entries for node-inputs. :foreign-libs remain useful for externs and maybe could be used to provide pre-bundled JS packages for the browser.
Hide
António Nuno Monteiro added a comment -

Attached patch with fix and test for initial review.

Show
António Nuno Monteiro added a comment - Attached patch with fix and test for initial review.
Hide
Thomas Heller added a comment -

This is far from completed but I'll not pursue this any further as I simply do not have the time.

Open questions:

  • What is the story for things that shouldn't run through the Closure Compiler? Specifically node.js builds?
  • What about relative requires? (eg "./something")
  • What about absolute requires? (eg "/lib/thing")
Show
Thomas Heller added a comment - This is far from completed but I'll not pursue this any further as I simply do not have the time. Open questions:
  • What is the story for things that shouldn't run through the Closure Compiler? Specifically node.js builds?
  • What about relative requires? (eg "./something")
  • What about absolute requires? (eg "/lib/thing")
Hide
David Nolen added a comment -

Thomas, we should have modified your original description since none of those things were ever under actually consideration.

Show
David Nolen added a comment - Thomas, we should have modified your original description since none of those things were ever under actually consideration.
Hide
Thomas Heller added a comment -

I get that I'm not great at communicating these things and shouldn't do brain-dumps in issues but the idea was only a basic sketch at the time. I'm not talking about the implications for :foreign-libs or :npm-deps now. I'm just talking about string require.

I did not have time to review the patch fully but I can tell you without running anything that {{(:require ["fs" :as fs])}} will not work, also {{(require '["react" :as react])}} will not work at the REPL.

As it stands now this feature is implemented incompletely with a bad example since this won't work for any actual node.js project.

Fine if you want individual tickets for every problem people will run into but I would have preferred if there was a little more discussion/review on this overall.

Show
Thomas Heller added a comment - I get that I'm not great at communicating these things and shouldn't do brain-dumps in issues but the idea was only a basic sketch at the time. I'm not talking about the implications for :foreign-libs or :npm-deps now. I'm just talking about string require. I did not have time to review the patch fully but I can tell you without running anything that {{(:require ["fs" :as fs])}} will not work, also {{(require '["react" :as react])}} will not work at the REPL. As it stands now this feature is implemented incompletely with a bad example since this won't work for any actual node.js project. Fine if you want individual tickets for every problem people will run into but I would have preferred if there was a little more discussion/review on this overall.
Hide
António Nuno Monteiro added a comment -

From my perspective, `:mpm-deps` was always about the browser. If you're compiling for Node.js just use `js/require`, no?

Am I missing some huge benefit here?

Show
António Nuno Monteiro added a comment - From my perspective, `:mpm-deps` was always about the browser. If you're compiling for Node.js just use `js/require`, no? Am I missing some huge benefit here?
Hide
Thomas Heller added a comment -

Consistency for one? How annoying is this from a users perspective that you can only use it for the browser but not when building node.js projects where you'd use many more string requires on average? If the answer is truly "Just use js/require" it would probably be much better to remove this feature altogether as :npm-deps already provided a solution for this otherwise. :npm-deps was just as rushed though.

Show
Thomas Heller added a comment - Consistency for one? How annoying is this from a users perspective that you can only use it for the browser but not when building node.js projects where you'd use many more string requires on average? If the answer is truly "Just use js/require" it would probably be much better to remove this feature altogether as :npm-deps already provided a solution for this otherwise. :npm-deps was just as rushed though.
Hide
António Nuno Monteiro added a comment -

String requires in `:npm-deps` is only about supporting cases like "react-dom/server". Nothing else.

Show
António Nuno Monteiro added a comment - String requires in `:npm-deps` is only about supporting cases like "react-dom/server". Nothing else.
Hide
Thomas Heller added a comment -

Not what I had in mind when I suggested this but if that is the case there is nothing left to say for me.

Show
Thomas Heller added a comment - Not what I had in mind when I suggested this but if that is the case there is nothing left to say for me.
Hide
David Nolen added a comment -

Thomas, like I said we should have updated the ticket. I also suggested string based require and it just didn't have anything to do with all this other stuff that you are suggesting in this issue. Supporting Node.js style paths as requires is just never, ever going to happen. Whatever you had in mind is not what I had in mind

Show
David Nolen added a comment - Thomas, like I said we should have updated the ticket. I also suggested string based require and it just didn't have anything to do with all this other stuff that you are suggesting in this issue. Supporting Node.js style paths as requires is just never, ever going to happen. Whatever you had in mind is not what I had in mind

People

Vote (0)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: