Fork me on GitHub
#cljs-dev
<
2017-06-06
>
juhoteperi06:06:59

String based require could also solve decoupling libraries from one library source if those requires also with with UMD modules

juhoteperi06:06:35

for example if (:require ["react" :as react :refer (createElement)]) would point createElement to js/React.createElement when using cljsjs/react

juhoteperi06:06:00

Then all the libraries could use this require form and it would point to whatever React is provided in users project

juhoteperi06:06:10

Though I guess mapping those refer etc. symbols to objects in window would be challenging

thheller08:06:06

so about string requires, I had some time to play with then now.

thheller08:06:21

for node they are very nice as they just work, if you run code through webpack they just work

thheller08:06:23

cljsjs packages could definitely make this easy for the browser as well

thheller08:06:58

otherwise it is a bit manual now and I don’t like that

thheller08:06:20

this is one part of it that webpack uses

thheller08:06:50

shadow-cljs will put all string requires it finds into a manifest.json in the :output-dir

thheller08:06:07

webpack uses that file and appends some stuff to the ui-bundle.js

thheller08:06:20

that isn’t very pretty but works

thheller08:06:40

cljsjs would be nicer and could do something similar

thheller08:06:51

just register the name it provides somehow (cljs.foreign/register "react" js/React) or so

juhoteperi09:06:31

It works for files that provide single object, but there are JS libs that don't do that

juhoteperi10:06:16

But if supporting those libs that only export single object is enough, foreign-libs could be extended to support this {:provides ["react"] :provides-object "React"}} or such, where :provides-object is the object that requires referring to the provided ns would point to

thheller12:06:57

@juhoteperi my thought was the there would be actual .cljs files that would “configure” what the provided JS files do

thheller13:06:22

so there would be a cljsjs/react.cljs that would (cljs.foreign/register "react" js/React)

thheller13:06:46

if there were multiple things to provide it could just call register again

juhoteperi13:06:29

I don't see what is the benefit in this compared to foreign-libs

thheller13:06:27

it isn’t .. there is none

thheller13:06:16

everything else is just a hack IMHO

juhoteperi13:06:38

I think that is going to be unnecessary once the first is implemented properly

thheller13:06:16

yes, I just want to stop this https://github.com/cljsjs/packages/pull/1192 from becoming a thing

thheller13:06:47

addressing it with a compiler option should be quicker than getting string requires

thheller13:06:09

changing every deps.cljs out there is also unlikely

juhoteperi13:06:36

There is clearly interest now to fix this so I'll keep that blocked for some weeks to see what solutions we can come up with

thheller13:06:35

it sucks that I can’t take my implementation for https://dev.clojure.org/jira/browse/CLJS-2061 into core but hopefully someone else can implement it somehow

thheller13:06:51

would also be neat if :foreign-libs could still be used as a fallback for the browser and :externs. it just works for node but requires work for the browser.

dnolen13:06:33

@thheller yeah just not interested in mapping to require and don’t care about Webpack integration at all

thheller13:06:54

uhm … ok …

thheller13:06:04

so you are against string requires?

dnolen13:06:09

@pesterhazy hrm I don’t know about surrogate package like that

thheller13:06:20

what else would you want to map to if not require?

dnolen13:06:22

@thheller no, I’m fine w/ string requries

dnolen13:06:45

the only thing we’re interested in at all is Google Closure stuff and Google Closure support for Node modules

dnolen13:06:49

we’re not doing our own thing

dnolen13:06:56

or integrating with anything else

thheller13:06:09

not sure I understand what you mean by that but I don’t have to if you agree with string requires

dnolen13:06:22

@juhoteperi I’m concerned that wouldn’t be enough anyway given many package freely use exports and don’t export one logical thing

thheller13:06:42

still some :foreign-libs changes are necessary for the transition

thheller13:06:55

but string requires are far more important

dnolen13:06:42

@pesterhazy that style of package shim is not what I’m getting at since old style :foreign-libs just create globals - you can’t really fix that now. Google Closure JS modules don’t.

pesterhazy13:06:03

@dnolen, I don't love surrograte packages either, but I'm trying to figure out what's the most pragmatic way to include unpatched reagent/om/... without pulling in cljsjs/react

dnolen13:06:17

so the shim really needs to create a ClojureScript namespace which manually exports the right thing depending on which dep you have

dnolen13:06:30

it’s annoying but I don’t really see this as affecting anything but real “base” libs

dnolen13:06:39

like React

pesterhazy13:06:06

so you would prefer a react surrogate package that includes empty namespaces instead of relying on foreign-libs?

dnolen13:06:08

people aren’t using random things from NPM so the old style :foreign-libs don’t really apply to them

dnolen13:06:25

@pesterhazy the namespace cannot be empty

dnolen13:06:28

you will have to def something

dnolen13:06:58

but I think you may not have understood what I was suggested yesterday

dnolen13:06:09

what I’m talking about needs 2 artifacts

dnolen13:06:27

one for the old way with CLJSJS and one for the new way - and people can control what they get with Maven coordinates

dnolen13:06:24

the point here is that end users now can just use NPM React without thinking about it or changing much

dnolen13:06:38

via Reagent/Rum/Re-frame/Om/etc which they cannot do today

dnolen13:06:28

then the problem of using some random NPM React Element goes away modulo externs

juhoteperi13:06:30

I'm just testing npm-deps and while React is parsed okay by Closure, create-react-class is not 😄

pesterhazy13:06:55

@dnolen, would we have to change Reagent/Om as well?

dnolen13:06:09

@pesterhazy that’s right but it would be a boring change - the best kind

dnolen13:06:25

just don’t refer to React directly, but through some CLJS namespace

juhoteperi13:06:09

Would these CLJS namespaces be written by hand or generated automatically?

thheller13:06:12

just (:require ["react" :as react]) and have some way for :foreign-libs or :npm-deps to provide that somewhere .. no need for cljsjs.alias mess

pesterhazy13:06:36

From working with react-native, I know that Reagent needs to be able to access React from window.React as well, because there we kind of have to co-operate with the react-native packager

pesterhazy13:06:06

so for that we'd have a shim library that grabs React from window.React, and exports it?

juhoteperi13:06:23

That would be pretty much what cljsjs/React does, that also uses window.React

dnolen13:06:28

@pesterhazy I’m sure there will be stuff like that - but doesn’t sounds like something for what I’m talking about to be concerned about

juhoteperi13:06:46

but there could be a third shim for react-native which doesn't include the browser JS file like Cljsjs/React, but also uses window.React

dnolen13:06:11

@juhoteperi it remains to be seen - first just want to see if it works - and then perhaps it can be sugared / automated

juhoteperi13:06:38

Right, sounds good

dnolen13:06:39

but I really don’t think so, this is about some JS package in a graph that everyone else is depending on

dnolen13:06:13

there tons of stuff in NPM that people want to use that are just leaf deps

dnolen13:06:38

so what I’m proposing doesn’t require any ClojureScript work at all

dnolen13:06:54

just demonstration that it’s viable

pesterhazy13:06:37

a change with little impact that makes it easy to use foreign js would be great

dnolen13:06:04

yes I suspect string based require + this pattern is all we need

pesterhazy13:06:04

could you explain what you meant by your comment on webpack above?

dnolen13:06:43

my comment was just that better integration with Webpack is a non-goal

dnolen13:06:04

we’re not going to support JS require in anyway that isn’t through Google Closure

dnolen13:06:42

this isn’t to criticize using Webpack right now to make this work

pesterhazy13:06:51

ah ok not supporting js/require makes a ton of sense

dnolen13:06:55

that’s the only option - and it’s understandable

pesterhazy13:06:42

the advantage of using webpack as a compnaion - to build a second bundle - is that you tap into a world of packages that get a lot of eyeballs

pesterhazy13:06:07

I mean, everyone in js land tests with webpack

dnolen13:06:04

sure but I don’t think it’s actually essential in the long run thanks to ES6

thheller13:06:16

ES6 doesn’t change anything as long as there are things like JSX and babel .. the JS world is just getting started with this mess

thheller13:06:33

only part ES6 solves are that import/exports are static

dnolen13:06:40

“only part”

dnolen13:06:43

that is huge

thheller13:06:55

I agree that is huge but not enough by a long shot

thheller13:06:17

if everyone just publishes ES6 that is fine

thheller13:06:31

time will tell if they do that

dnolen13:06:23

we don’t need “everyone” to do this to benefit

dnolen13:06:49

we piggieback heavily on the React ecosystem and that’s an ecosystem jumping on the ES6 bandwagon

thheller13:06:24

node with .mjs should help as well .. but all this is going to take time … I personally don’t want to wait for that

thheller13:06:41

so require can work today without much work

thheller13:06:24

really only needs string requires to solve almost every issue if we also can get rid of the pseudo namespaces in cljsjs packages

juhoteperi14:06:56

When a module (`create-react-class`) exports only a single function as the the object, is there a way to refer to that with current npm-deps?

juhoteperi14:06:59

Okay. Now looks like browser just can't load React because it tries to read process.ENV.NODE_ENV which is not available

spinningtopsofdoom14:06:16

@juhoteperi Try adding this to your project

(ns process.env)

(goog-define NODE_ENV "development")

juhoteperi14:06:44

I added this to index.html for now: var process = {env: {NODE_ENV: "development"}};

dnolen14:06:22

@juhoteperi yes that pattern is supported now

dnolen14:06:33

check the git logs, Antonio worked on that

juhoteperi14:06:45

Yes, got that working, now trying to use react-dom/server

dnolen14:06:12

right that won’t work w/o string require

dnolen14:06:45

or at least I don’t think it will

juhoteperi14:06:51

Any workarounds? JS file which provides react-dom-server and exports require("react-dom/server")?

juhoteperi14:06:12

I thought the problem would be to just require it but I guess the file is not even indexed by node-inputs: https://github.com/clojure/clojurescript/blob/ccebc81419a9611e9521c2741c69851eebf327c0/src/main/clojure/cljs/closure.clj#L2115

dnolen15:06:05

@juhoteperi not indexed because we only index things explicitly required by the build

dnolen15:06:23

pretty sure there isn’t a workaround until string require

juhoteperi15:06:27

That is not based on requires but npm-deps

dnolen15:06:16

the NPM feature does actually crawl the dep graph

dnolen15:06:28

index modules just about the directories

dnolen15:06:34

not what files we actually pass to Google Closure

juhoteperi15:06:37

NPM feature crawls dep graph for just those modules required by package index files

juhoteperi15:06:42

e.g. react-dom/index.js

juhoteperi15:06:47

but it skips react-dom/server.js

juhoteperi15:06:21

Even if there was support for (:require ["react-dom/server"]) a change would also be needed to index react-dom/server.js because current code doesn't include that file in foreign-libs

deas15:06:03

@juhoteperi Instead of using :npm-deps, you can build up :foreign-libs yourself by calling node-inputs add whatever is missing for you. This is what I do to get the react-dom.server ns.

juhoteperi15:06:32

Except for single function export of create-react-class it would be possible to create shim react and react-dom cljs namespaces which refer to window.React and window.ReactDOM

juhoteperi15:06:11

And I guess it would be as easy to create shim which refers to require("react") for Node

deas15:06:53

@juhoteperi There is a npm-deps branch? Good to know. Thanks! 🙂

juhoteperi15:06:35

Just created it. Not going to be merged as is, just for testing

juhoteperi16:06:05

Got the advanced build also working. Reagent demo site down from 462KB to 434KB (non-gzipped).