Fork me on GitHub
#cljs-dev
<
2019-05-15
>
kommen13:05:09

@mfikes is it ok to assign you cljs jira tickets to review?

kommen14:05:44

thanks, done

mfikes14:05:56

From the big picture, I tend to try to do the mechanical stuff (CI, etc), and other review stuff, to help get it in shape and make it easier for David to ultimately take a decision on any given patch.

mfikes14:05:18

I'm happy to play the role of GitHub CI robot for JIRA. 😝 But I also actually look at the contents of patches as well. 🙂

kommen14:05:43

I’m experimenting with moving our server side rendering of our re-frame app from nodejs to graaljs, and since you did most (all?) of the graal work it seemed sensible to ask you for feedback. but I feel better to ask before assigning “work” to somebody 🙂

mfikes14:05:29

Ahh, right. I'm effectively responsible for the Graal.js REPL, good and bad. I can't punt that one.

lread15:05:43

Hiya! I’m digging into https://dev.clojure.org/jira/browse/CLJS-3079 (Explore Keeping ClojureScript Versioning out of Git Diffs) and am getting the feeling that version fixups in src/main/cljs/cljs/core.aot.js and src/main/cljs/cljs/core.cljs.cache.aot.edn (https://github.com/clojure/clojurescript/blob/47386d7c03e6fc36dc4f0145bd62377802ac1c02/script/build#L57) are no-ops and no longer needed - but I thought best to double check with those more in the know before turfing.

lread15:05:20

Also noticed that https://github.com/clojure/clojurescript/blob/master/script/revision is stale and am wondering if it is still relevant.

dnolen16:05:42

I finally got a good idea about how to handle transitive dependencies on Node.js from a ClojureScript library

🚀 4
dnolen16:05:01

feedback welcome, but I'm feeling pretty good about this and will probably start working on it shortly ^

mfikes17:05:47

Maybe this interacts with, or provides a solution to https://dev.clojure.org/jira/browse/CLJS-3082

mfikes17:05:45

While the primary goal of this proposal is an enabler when creating libraries, it also seems to perhaps address some of https://clojurians.slack.com/archives/C07UQ678E/p1557760534351700

mfikes17:05:39

It also seems like there would be nothing stopping this from panning out for self-hosted ClojureScript. In concrete terms Lumo and Planck already support :foreign-libs and you could see them being updated to support the new semantics.

mfikes17:05:19

As a secondary concern to that proposal: Perhaps caching considerations. If all the generated stuff gets dropped into a target directory, maybe there's some way to know that the :bundle-fn need not be run again if already done. :thinking_face:

thheller18:05:08

@dnolen can you expand a bit on what this map is actually supposed to express?

{cljs-react {React "react"}}
{cljs-foo {* "foo"}}
{cljs-bar {[Foo Baz] "bar"}}

thheller18:05:18

I can't quite figure out what this is supposed to represent

mfikes18:05:42

My guess was: The keys are the synthetic foreign lib names. The vals are ways of expressing JavaScript imports in index.js

import React from 'react';
import * from 'foo';
import { Foo , Baz } from 'bar';
Interested in seeing an example of the files generated from this map...

mfikes18:05:22

That would lead to

window.React = React;
;; what goes here for the * ?
window.Foo = Foo;
window.Baz = Baz;
but I don't know about the * bit. I've climbed too far out on the speculative branch.

thheller18:05:25

and how does the CLJS side look? (:require [cljs-react])?

mfikes18:05:53

Yeah, my guess is that this is only a fancy way to construct foreign libs. But curious about the "reshaping" aspect and perhaps how such a map could be used to construct the desired namespace object. I guess we really need David to elaborate and make this aspect concrete.

lilactown18:05:03

import * from 'foo' isn't valid syntax AFAIK. you have to give it a name

👍 4
thheller18:05:40

that name doesn't really matter and the compiler can choose it

thheller18:05:54

import * as SOME$COMPILER$NAME from "react"

mfikes18:05:54

Yeah, thinking the same. A gensym

mfikes18:05:23

I think we've successfully reverse engineered David's map. 🙂

thheller18:05:37

still unsure about the CLJS side of things.

mfikes18:05:58

window.SOME$COMPILER$NAME = SOME$COMPILER$NAME;

thheller18:05:58

things are working quite well in shadow-cljs using string requires so I don't know why we need a different system there

thheller19:05:04

I also outlined why shadow-cljs doesn't use webpack and this seems kinda relevant here since the gist mentions creating ONE index.js which completely discards code-splitting https://code.thheller.com/blog/shadow-cljs/2018/06/15/why-not-webpack.html

thheller19:05:26

happy to create an example repo for something that is not currently possible with webpack

thheller19:05:42

so I'd consider this a far more important issue to fix first

dnolen19:05:05

The gist doesn’t mention one index.js - code splitting is considered in the above choices

dnolen19:05:44

Not really concerned about what shadow does here nor why it didn’t use webpack

dnolen19:05:05

Since webpack doesn’t matter here at a fundamental level

dnolen20:05:20

Comments I’m looking for should be about identifying problems that might arise when using this in complex projects with transitive deps

dnolen20:05:38

Since that’s the problem I’m actually interested in

thheller20:05:37

but which problem are you actually trying to solve that (:require ["react" :as r]) doesn't already solve?

dnolen20:05:18

Transitive deps - what does that have to do with the problem?

dnolen20:05:58

Anyways let’s leave shadow out of this please

dnolen20:05:46

Not interested in anything to do with the ns form - not under consideration for this

thheller20:05:17

changes? its already all supported?

thheller20:05:41

the issue as far as I can tell is that CLJS code wants to depend on JS code

thheller20:05:52

and the compiler needs a way to figure out where this JS is coming from

thheller20:05:05

ie. cljsjs style foreign-libs or webpack

thheller20:05:08

so the problem is now expressing dependencies

thheller20:05:46

but that can't be expressed universally if someone prefers to use cljsjs.react over react via webpack/npm

dnolen20:05:15

Transitive dependencies from npm is not supported

dnolen20:05:15

So I don’t know what you’re talking about

thheller20:05:42

yeah ... I'm lost.

thheller20:05:43

> Allow users to create deps.cljs file in their artifact that expresses which Node dependencies they need.

thheller20:05:06

in code (:require ["react" :as r]) + deps.cljs :npm-deps {"react" "..."}

thheller20:05:24

the compiler can generate all other needed stuff from that

thheller20:05:23

I'm missing the point where more info is required?

dnolen20:05:18

That information is meaningless

dnolen20:05:25

Since we don’t use it

dnolen20:05:45

And we’re not going to build that stuff into ClojureScript for now

thheller20:05:03

what are you talking about? its already all in there?

dnolen20:05:05

We don’t build npm foreign deps!

dnolen20:05:16

That thing is a stub

thheller20:05:03

I'm lost. I'm looking at this from the compiler perspective

thheller20:05:50

analyzer runs into (:require ["react" :as r]) ... seems the string it doesn't recognize, collects that string and picks a "static" alias like module$react and uses this in the code.

thheller20:05:01

once compilation is finished it takes all the references it created

dnolen20:05:34

well I can confirm we're talking past each other

thheller20:05:37

and ask the :bundle-fn to please provide window.module$react which was "react"

dnolen20:05:46

in the foreign deps scenario, those entries are stubs

dnolen20:05:55

there is no graph, because we have no idea what it is

dnolen20:05:15

we don't look at the JS graph - so there is no graph

thheller20:05:37

what graph? I didn't use the word graph?

thheller20:05:06

I created one alias and that alias is filled in later

thheller20:05:13

by whatever means necessary.

dnolen20:05:30

what I'm saying is those deps that we don't know anything about

thheller20:05:31

window.module$react = React when remapping cljsjs.react :global-exports

dnolen20:05:39

there's nothing we can do w/ what thing

dnolen20:05:47

or those thing(s)

thheller20:05:57

or window.module$react = require("react") when using webpack

dnolen20:05:50

to clarify my point above - at the ns level not interested because we don't want to deal with JS import-isms

dnolen20:05:55

we've avoided that, and will continue to do so

dnolen20:05:16

nothing's changing here and I don't walk to repeat these old conversations

thheller20:05:54

can you clarify what you mean please? I don't know what you mean by "... we don't want to deal with JS import-isms"

dnolen20:05:18

modules as functions, default etc.

dnolen20:05:59

anyways just toss that conversation out the window - we're not going over this again

dnolen20:05:26

anyways setting that stuff aside

thheller20:05:27

going over what? if you are offended by the string pretend I was using (:require [react :as r])? there needs to be something in the ns to require right?

dnolen20:05:58

no I'm just saying you're not talking about what I'm proposing yet far as I can tell

dnolen20:05:41

what I'm talking about and the only thing I'm talking about is you can't depend on some CLJS library which uses something from NPM w/ defining how to build those deps downstream

thheller20:05:17

please clarify then what this means? {cljs-react {React "react"}}

thheller20:05:23

what is the cljs-react?

dnolen20:05:41

nothing new

dnolen20:05:48

nothing has changed, it's exactly what it is right now

thheller20:05:21

so it is the require alias right? the user would (:require [cljs-react ...])?

dnolen20:05:41

yes, like I said there's nothing new to see here

dnolen20:05:57

you can pick whatever name you like - as before

thheller20:05:41

{React "react"} what is this then?

dnolen20:05:53

what do you want to extract and how do you want to reshape

dnolen20:05:56

also nothing new

dnolen20:05:10

you were doing this before when writing boilderplate index.js

thheller20:05:36

so you want to replicate import React from "react"?

dnolen20:05:58

it's just boilerplate - we can generate that thing for you

dnolen20:05:13

but there's no change in the existing approach

dnolen20:05:18

no new design decisions

dnolen20:05:50

the only thing that's new that's worth talking about IMO, is :bundle-fn :bundle-args the new stuff

dnolen20:05:04

code splitting is trivial since you can invoke :bundle-fn multiple times - no big deal

thheller20:05:04

I'm telling you it is not but if you have a magic recipe I'd be very curious to see that

thheller20:05:55

but getting back to {cljs-react {React "react"}}

thheller20:05:03

the React name is actually not relevant

thheller20:05:16

import module$react from "react" is perfectly valid

thheller20:05:25

so {cljs-react "react"} would be fine

thheller21:05:04

since we can also pick cljs-react we are left with "react"

dnolen21:05:28

right that's just a typo, fixed

dnolen21:05:37

@thheller ah yeah, I see the problem w/ trying to code split w/ multiple bundle calls - but you can still have webpack code split w/ multiple entry points it seems + dedupe

thheller21:05:29

I couldn't figure it out .. and I tried for quite a while

thheller21:05:43

fundamentally webpack deals with code splitting very differently

dnolen21:05:43

you mean the official thing doesn't work?

thheller21:05:48

I haven't checked in a while and maybe it works with webpack4/5 but back then it organized code in really strange ways

thheller21:05:07

and most importantly it uses dynamic import() which mean everything becomes async

dnolen21:05:30

@thheller ^ this appears to show that like Closure you can load the chunk yourself

dnolen21:05:49

I don't see why you need to use import

thheller21:05:30

as I said .. I couldn't figure it out ... maybe I missed something obvious but all my attempts failed

thheller21:05:40

by either including dependencies multiple times or in the wrong chunks

dnolen21:05:33

yeah the newer plugin appears to be better? but who knows - as long as it's relatively reasonable - this is not a big concern

martinklepsch21:05:57

Just following along here. Is the misunderstanding that David is trying to figure out how to allow cljs libraries to declare dependency on some JS module (presumably provided via a webpack bundle) so that downstream consumers don't have to provide it themselves and Thomas is thinking more in the general case of using deps from npm?

thheller21:05:29

I think it is fine to use (:require ["that-npm-package" :as x]). David does not seem to agree and instead expects (:require [some-alias :as x]) + {some-alias "that-npm-package"} declared in deps.cljs

martinklepsch21:05:12

If I would use a library that has a deps.cljs with the following, would I require React as (:require [cljs-react :as react])?

{cljs-react "react"}
{cljs-foo {* "foo"}}
{cljs-bar {[Foo Baz] "bar"}}
How would that be affected by a deps.cljs mapping "react" to a different global export?

martinklepsch22:05:48

@thheller right, but fundamentally that's not what David is even thinking about as far as I understand. With shadow-style string requires the problem of transitive depenencies is equally unsolved. Like if I write a library that depends on react how would I communicate that to a downstream consumer?

dnolen22:05:00

@martinklepsch good question - what do you mean different global export in this context?

thheller22:05:24

:npm-deps solves expressing the dependencies

dnolen22:05:35

but not how to build

dnolen22:05:39

so it's not useful

martinklepsch22:05:59

Well, say deps.cljs of lib A has:

{cljs-react "react"}
{cljs-foo {* "foo"}}
{cljs-bar {[Foo Baz] "bar"}}
and deps.cljs of lib B has:
{other-cljs-react "react"}
{cljs-foo {* "foo"}}
{cljs-bar {[Foo Baz] "bar"}}

dnolen22:05:05

if clj did this instead we'd still have the same problem

thheller22:05:51

@martinklepsch the alias doesn't matter since you can just dedupe on the package name

martinklepsch22:05:54

I mean it could also look like: deps.cljs of lib A has:

{cljs-react "react"}
{cljs-foo {* "foo"}}
{cljs-bar {[Foo Baz] "bar"}}
deps.cljs of lib B has:
{cljs-react {* "foo"}}
{cljs-bar {[Foo Baz] "bar"}}
that could break things presumably?

thheller22:05:38

well yeah that would be a problem

thheller22:05:02

having two names for things is fine

martinklepsch22:05:25

@dnolen I guess I'm missing a more complete example to really understand how a full deps.cljs would look like and how it would affect behavior in an upstream project consuming those libraries depending on some JS bundles being present

martinklepsch22:05:47

Just to make sure I'm not on the wrong track entirely. This is about a cljs library like cljs-time depending on moment.js and properly indicating that dependency to an upstream consumer (e.g. an application or another library) — is that right?

dnolen22:05:06

@martinklepsch that above case isn't a problem

dnolen22:05:14

we don't allow multiple versions of dep

dnolen22:05:20

right hand side is unique

dnolen22:05:28

the generated name doesn't really matter too

dnolen22:05:52

ah yeah @thheller is exactly right

martinklepsch22:05:53

I'd love to see the code snippet in the proposal expanded to a full deps.cljs to display a typical case like the one I tried to describe above. Right now I'm not fully sure where this is going and how it relates to the rest of the libraries deps.cljs file. (All of this just feedback/trying to understand things btw, no expectations etc)

thheller22:05:40

in my world the deps.cljs contains {:npm-deps {"react" "some-version"}} and nothing else

thheller22:05:57

all other build related aspects can be derived from (:require ["react" :as x])

thheller22:05:41

thats how it works in shadow-cljs. not sure what benefit the additional alias via :foreign-libs provides over just making things more difficult

thheller22:05:18

I can understand that strings aren't "pretty" but it is how the javascript world works

thheller22:05:47

ie. clojure has special :import syntax for java classes which is rarely used in CLJS

thheller22:05:54

so to me thats just embracing the host

martinklepsch22:05:11

Backwards compatibility might be one factor but I don't fully understand all the nuances. In general it seems like this has been considered and decided against so more messages in an ephemeral chat room seem unlikely to change anyone's mind. Maybe at this point a proper writeup comparing the different approaches, each with their benefits and tradeoffs might be one of the few ways to move this discussion forward...

martinklepsch22:05:12

Obviously this is more work for everyone and no one can expect that from the other 🙂

dnolen22:05:16

really all the stuff about the ns form is not on or off the table

dnolen22:05:24

but it's really not that important can be done later if at all

thheller22:05:32

what the heck is going on .. the string require support was added ages ago

thheller22:05:48

NOTHING needs to be added

dnolen22:05:00

wasn't specific to node has no semantics around node

thheller22:05:21

what does that have to do with anything?

dnolen22:05:44

that you keep on about this thing that's not important

martinklepsch22:05:45

@thheller well, then I apparently have no idea what I'm talking about 🤡

martinklepsch22:05:11

Alright folks, I'll go to bed. You two should have a beer at some point 🙂 🍻

martinklepsch22:05:32

I heard there's an awesome conference in Europe happening this summer 😉 😄

thheller22:05:51

apparently the :missing-js-modules stuff was removed for some reason but that would have been all the info you'd need to make everything work

thheller22:05:10

but I can understand not wanting to do string requires and just having to alias everything manually

dnolen23:05:52

@thheller there's actually another subtle thing to consider that I'm uncomfortable making a hard decision on now

dnolen23:05:21

that there's an ambiguity if you need both foreign-deps and :npm-deps at the same time, we've run into a case at work where this is the case

dnolen23:05:13

so :global-exports lets you pick a different name to avoid that clash - nothing you can currently do about the :npm-deps case

dnolen23:05:07

you need foreign libs for some things, but need to generate for node requires for other stuff

dnolen23:05:28

because maybe you can't get to all of NPM but some subset for N reasons

dnolen23:05:06

so sticking w/ what we have satisfies that for now w/o changes in that part

dnolen23:05:39

but I can't imagine we will be the only people that run into this problem if you're doing stuff with Node