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

@kommen Definitely.

thanks, done

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.

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. 🙂

🙂

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 🙂

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

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.

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

https://gist.github.com/swannodette/ebd8b65f887318ba68579b6bea911daf

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

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

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

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

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.

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:

@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"}}

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

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';

That would lead to

window.React = React;
;; what goes here for the * ?
window.Foo = Foo;
window.Baz = Baz;

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

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.

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

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

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

Yeah, thinking the same. A gensym

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

still unsure about the CLJS side of things.

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

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

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

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

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

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

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

Since webpack doesn’t matter here at a fundamental level

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

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

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

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

Anyways let’s leave shadow out of this please

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

changes? its already all supported?

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

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

ie. cljsjs style foreign-libs or webpack

so the problem is now expressing dependencies

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

Transitive dependencies from npm is not supported

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

yeah ... I'm lost.

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

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

the compiler can generate all other needed stuff from that

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

That information is meaningless

Since we don’t use it

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

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

It is not

We don’t build npm foreign deps!

That thing is a stub

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

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.

once compilation is finished it takes all the references it created

well I can confirm we're talking past each other

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

in the foreign deps scenario, those entries are stubs

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

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

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

I created one alias and that alias is filled in later

by whatever means necessary.

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

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

there's nothing we can do w/ what thing

or those thing(s)

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

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

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

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

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"

modules as functions, default etc.

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

anyways setting that stuff aside

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?

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

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

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

what is the cljs-react?

nothing new

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

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

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

you can pick whatever name you like - as before

{React "react"} what is this then?

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

also nothing new

you were doing this before when writing boilderplate index.js

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

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

but there's no change in the existing approach

no new design decisions

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

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

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

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

the React name is actually not relevant

import module$react from "react" is perfectly valid

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

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

right that's just a typo, fixed

@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

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

fundamentally webpack deals with code splitting very differently

you mean the official thing doesn't work?

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

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

https://webpack.js.org/guides/lazy-loading/

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

I don't see why you need to use import

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

by either including dependencies multiple times or in the wrong chunks

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

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?

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

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"}}

@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?

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

:npm-deps solves expressing the dependencies

but not how to build

so it's not useful

Well, say deps.cljs of lib A has:

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

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

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

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

having two names for things is fine

@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

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?

@martinklepsch that above case isn't a problem

we don't allow multiple versions of dep

right hand side is unique

the generated name doesn't really matter too

ah yeah @thheller is exactly right

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)

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

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

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

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

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

so to me thats just embracing the host

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...

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

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

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

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

NOTHING needs to be added

wasn't specific to node has no semantics around node

what does that have to do with anything?

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

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

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

https://github.com/clojure/clojurescript/commit/dd5d86223f359e06434a152ad0ea5e0bee317459

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

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

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

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

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

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

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

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

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