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

[RFC] Better support for importing non-closure code #10

Closed
thheller opened this Issue May 17, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@thheller
Copy link
Owner

thheller commented May 17, 2017

This is an experiment for a new CLJS feature that would map npm packages to CLJS namespaces automatically.

Goal

Instead of writing this

(ns demo.browser)

(def react (js/require "react"))
(def rdom (js/require "react-dom"))
(def render (.-render rdom))
(def Component (.-Component react))

(defn foo []
  (.createElement react "h1" nil "hello from react"))

(render (foo) (js/document.getElementById "app"))

shadow-cljs now supports writing

(ns demo.browser
  (:require ["react" :as react :refer (Component)]
            ["react-dom/server" :as rdom :refer (render)]))

(defn foo []
  (react/createElement "h1" nil "hello from react"))

(render (foo) (js/document.getElementById "app"))

Implementation

This is achieved by creating a pseudo resource that mimics the namespace.

goog.provide("shadow.npm.react")

// for npm/node targets
shadow.npm.react = require("react");

// for browser targets
shadow.npm.react = window["npm$modules"]["react"];

For browser targets a tool like webpack must be used to properly create the
window["npm$modules"] data.

This can be as simple as creating a bundle.js and running webpack bundle.js dist/bundle.js and including the generated file before the CLJS compiled output.

var x = window["npm$modules"] = {};
x["react"] = require("react");
x["react-dom"] = require("react-dom");

The upside is that this is fully compatible with the Closure Compiler and webpack -p since they don't know about each other just like :foreign-libs.

Edits

  • removed the original idea of creating automatic aliases like npm.react based on feedback. There should not be special namespace magic.
@shaunlebron

This comment has been minimized.

Copy link
Contributor

shaunlebron commented May 17, 2017

Exciting to see more thoughts on the js<->cljs interfacing through webpack!

To summarize, CLJS from JS...

require('shadow-cljs/foo.core');

And this new proposal is to apply a direct analogue for JS in CLJS...

(ns (:require [npm.foo]))

or to add a new ns grammar if an npm pseudo-namespace feels less idiomatic:

(ns (:npm-require "foo")) ;; or (:npm-require foo)?

questions:

Could you clarify the import class issue? I don't understand how cljs's import differs from require-refer in implementation and what's really happening. Does this have to do with dealing with namespaces when child vars are in separate files?

Would :npm/require avoid the class issue? And is there a benefit of using this over js/require, other than idiomatic placement inside ns?

@thheller

This comment has been minimized.

Copy link
Owner Author

thheller commented May 17, 2017

As @darwin pointed out in Slack the automatic aliasing is "magic" and that is not good.

I like the alternate suggestion of expanding the ns form that just uses a string just like you would with js/require.

(js/require "some/thing") becomes (:js/require ["some/thing" :as foo :refer (Bar)]). Other suggestions were :require-native or :require-foreign.

@thheller

This comment has been minimized.

Copy link
Owner Author

thheller commented May 17, 2017

@shaunlebron for goog files we know where the names come from. We then know which file goog.net.XhrIo.ResponseType is in. The same is not easily available for npm packages.

The :js/require approach would also fix the class issue.

The benefit I see is that we can provide a little syntax sugar to avoid too much native interop code. We can also analyze this statically and wouldn't need to run through the code to find requires. This should help with tool interop (ie. webpack config generation).

@thheller thheller changed the title [RFC] Automatic npm namespace mapping [RFC] Better support for importing non-closure code May 17, 2017

@thheller

This comment has been minimized.

Copy link
Owner Author

thheller commented May 17, 2017

David Nolen:

I honestly don’t have a problem with supporting strings in :require instead of just symbols

There is hope.

@thheller

This comment has been minimized.

Copy link
Owner Author

thheller commented May 18, 2017

I just completed by first successful test using the new mechanism.

(ns demo.browser
  (:require ["react" :as react]
            ["react-dom" :as rdom]))

(defn foo []
  (react/createElement "h1" nil "hello from react"))

(rdom/render (foo) (js/document.getElementById "app"))
  • :refer doesn't work since CLJS complains that the var doesn't exist
(ns demo.browser
  (:require ["react" :as react]
            ["react-dom" :as rdom :refer (render)]))

;; Invalid :refer, var shadow.npm.react_dom/render does not exist

Need to look into the resolve-var code, should be an easy fix though.

Means that it will generate "bad" code

(shadow.npm.react_dom.render.cljs$core$IFn$_invoke$arity$2 ? shadow.npm.react_dom.render.cljs$core$IFn$_invoke$arity$2(G__89996_89998,G__89997_89999) : shadow.npm.react_dom.render.call(null,G__89996_89998,G__89997_89999));

but should generate

shadow.npm.react_dom.render(G__89996_89998,G__89997_89999);

The code still works but going through the .call every time is bad for performance.

@thheller

This comment has been minimized.

Copy link
Owner Author

thheller commented May 29, 2017

Created: https://dev.clojure.org/jira/browse/CLJS-2061

I will not be pursuing the implementation for CLJS itself due to lack of time.

It works fine in shadow-cljs but I think code that only compiles with shadow-cljs is harmful and I will not be recommending this feature until there is a official consensus that this will be coming to CLJS. I'll revisit the implementation to match what CLJS does if necessary.

@thheller thheller closed this May 29, 2017

@jiyinyiyong

This comment has been minimized.

Copy link
Collaborator

jiyinyiyong commented May 31, 2017

I got a question, normally we write :refer with []:

(:require [a :refer [b c]])

Why it's () for npm?

(:require ["a" :refer (b c)])
@thheller

This comment has been minimized.

Copy link
Owner Author

thheller commented May 31, 2017

[] should work just the same as (). I just do () out of habit but the compiler doesn't care which one you use as longs as its sequential.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.