Fork me on GitHub
#cljs-dev
<
2017-07-06
>
dnolen00:07:11

@juhoteperi so what are you suggesting?

dnolen00:07:38

parse, if has errors, don’t proceed and report failures

dnolen00:07:48

and after es6 rewrite also report failures?

anmonteiro00:07:43

oh hrm I guess there’s context in the issue

anmonteiro00:07:53

util/relative-name is still not quite right. I’m fixing & adding test cases

anmonteiro00:07:22

actually can’t really add tests because cross-platform 😞

anmonteiro00:07:45

@dnolen hopefully last round of fixes for NPM deps & JS modules on Windows https://dev.clojure.org/jira/browse/CLJS-2177

anmonteiro00:07:50

had to spawn a Windows VM 🙂

anmonteiro00:07:59

this was getting under my skin

dnolen00:07:23

@anmonteiro left a question on the issue

anmonteiro00:07:40

I tested the NPM deps thing in a folder that had spaces on purpose

dnolen00:07:51

@anmonteiro one last question on the issue

anmonteiro01:07:19

OK hold on, string-based requires don’t work because of a backslash issue too

anmonteiro01:07:29

putting that in the same patch

anmonteiro01:07:10

@dnolen OK fixed that too

anmonteiro01:07:24

FYI, the change now is to replace the paths in the module graph :provides with forward slashes so people can require e.g. "react-dom/server" and have that work on Windows too

dnolen01:07:09

@anmonteiro ok, will look at that in the new patch

anmonteiro01:07:24

it’s the one-line change in module_deps.js

anmonteiro01:07:52

(I replaced the patch with the new one, if that wasn’t clear)

dnolen01:07:18

the attached patch doesn’t look different to me

dnolen01:07:22

only 2 files changed

anmonteiro01:07:31

you’re right

anmonteiro01:07:35

where did it go wrong

anmonteiro01:07:14

@dnolen fixed now, sorry

anmonteiro01:07:33

I keep outputting patches to the samples directory when I’m running the string-based requires example

dnolen01:07:55

looks ok, I don’t suppose we have any npm deps tests?

anmonteiro01:07:16

let me create a ticket to add tests for that

dnolen01:07:23

cool thanks

anmonteiro15:07:00

@dnolen the follow up to 2180 is to make sure that this string gets added to cljs_base: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/closure.clj#L1275

anmonteiro15:07:13

I couldn't get it to work without any terrible hacks

anmonteiro15:07:41

I think the module graph doesn't know to assign it to a module

anmonteiro15:07:32

That causes a whitespace bundle to to a request for cljs_deps.js when one does not exist

dnolen16:07:58

@anmonteiro just seems like a bug, cljs.modules-graph assumes ijs is a map - file an issue

dnolen16:07:10

it should work without hacks, cljs.modules-graph should handle that case correctly

dnolen16:07:20

easy to write a test case for

anmonteiro16:07:54

@dnolen I’m happy to fix but need more info. should it handle the case that source is a string or should we pass it an ijs?

anmonteiro16:07:22

asking because I tried passing ijs but didn’t know what to put in :provides or :url and throws without it

dnolen16:07:20

@anmonteiro well we have to compute something for that via deps/-provides

dnolen16:07:45

previously inputs would be added to base in dep order (since inputs already sorted)

dnolen16:07:02

strings don’t need :url

dnolen16:07:18

just need :provides

dnolen16:07:51

so just need to give it a :provides and I think module graph can handle it

dnolen16:07:17

and that :provides just needs to match deps/-provides

anmonteiro16:07:33

wouldn’t e.g. cljs.core need to depend on it though for it to be put at the beginning of the bundle?

dnolen16:07:52

don’t think so since it wasn’t necessary before

dnolen16:07:58

we just put in as the first compiler input

anmonteiro16:07:06

gotcha, let me try to make it work

dnolen16:07:47

so map over inputs before calling expand-modules to fix up string compiler inputs

dnolen16:07:59

maybe that’s all you need to do

anmonteiro16:07:19

“fix up string compiler inputs” means making ijs out of them?

dnolen16:07:39

but only for expand-modules

dnolen16:07:01

as long as the :provides name in the expanded modules graph is the same as the one we compute later is the same we’re good

dnolen16:07:10

just SHA the string to get the name

anmonteiro16:07:35

like we do for some node inputs

dnolen16:07:58

you may need to change the javascript-name case for String

dnolen16:07:35

yeah -provides for String would try to parse-js-ns

dnolen16:07:00

would would probably return nil

dnolen16:07:11

yeah I guess this is probably a bug - strings can provide nothing

dnolen16:07:20

but then you can’t index them for module assignment

anmonteiro16:07:40

right, so do we need a different strategy?

dnolen16:07:59

no I think we should fix the bug 🙂

dnolen16:07:10

strings should have :provides always

dnolen16:07:20

and then this will work

dnolen16:07:53

line 376 cljs.closure should fall back to SHA of string for the name

dnolen16:07:34

when you call expand modules you can convert to map using this

dnolen16:07:56

probably want to do something like js_source_string_SHA

dnolen16:07:44

the ijs of a string will just be {:provides ["js_source_string_SHA"]}

dnolen16:07:52

you don’t need anything else I think - sufficient to make this work

anmonteiro16:07:57

we use this all over the place, probably worth making a helper

(->> (util/content-sha js)
           (take 7)
           (apply str))

anmonteiro16:07:21

thanks, let me try and make it work 👍

dnolen16:07:39

or tweak content-sha to take length or something

anmonteiro17:07:13

hrm this doesn’t work for only expand-modules

anmonteiro17:07:33

also need to tweak it for module-graph/find-sources-for-module-entry

anmonteiro17:07:55

@dnolen I think we can’t be touching string inputs, and just make cljs.module-graph operate on deps/-provides instead of assuming everything is a map?

dnolen17:07:47

I don't want module-graph to depend on the protocols

dnolen17:07:03

One day I want to get of that stuff

dnolen17:07:08

Really annoying

anmonteiro17:07:16

I suppose we’ll have to give it a URL then

anmonteiro17:07:22

when coercing from string input to a map

anmonteiro17:07:44

which is the problem I was having yesterday

dnolen17:07:26

One day I want everything to be maps

dnolen17:07:34

This coercion stuff is not good

anmonteiro18:07:49

open to better ideas to solve the problem if you have any

anmonteiro20:07:35

@dnolen do you see any drawbacks of using the new resolve macro when not code splitting?

dnolen20:07:13

not really

anmonteiro20:07:44

@dnolen something I noticed too, the resolve macro expects the symbol to be quoted. is this a hard requirement?

anmonteiro20:07:02

we could easily allow both (resolve 'foo.bar/core) and (resolve foo.bar/core)

anmonteiro20:07:18

or at least warn/throw for the case we don’t support

dnolen20:07:16

but I don’t see a reason to allow both

dnolen20:07:20

the later doesn’t work in Clojure

dnolen20:07:33

I agree about a better error

anmonteiro20:07:42

that’s what I’d go for too

anmonteiro20:07:28

@dnolen https://dev.clojure.org/jira/browse/CLJS-2182 also noticed that the other macros that have assertions for guarding against this behavior will error before getting there because of destructuring in arguments

anmonteiro20:07:34

patch welcome to fix those cases?

anmonteiro20:07:08

current errors are cryptic, when with simple fixes we can provide the more accurate “arguments should be quoted symbols” message

dnolen20:07:36

@anmonteiro patch welcome - yeah there are quite a few macros that must take a quoted symbol and they all suffer from this problem

anmonteiro21:07:09

@dnolen also noticed we don’t have a ns-publics macro. For compat with Clojure does it make sense to add one?

dnolen21:07:39

I’m ok with that.

anmonteiro21:07:47

adding it in another ticket

anmonteiro21:07:13

along with ns-imports

dnolen21:07:40

sounds good

anmonteiro23:07:58

I wonder how hard it would be to support nested requires in ClojureScript

dnolen23:07:42

but what for? 🙂

anmonteiro23:07:13

very useful for namespaces like foo.bar foo.bar.baz to do like in Clojure (require '[foo.bar [baz])

anmonteiro23:07:25

or whatever the syntax is

dnolen23:07:46

oh that thing - I’d rather not

dnolen23:07:00

I’m pretty sure pretty much everyone one that worked on that considered it a misfeature

dnolen23:07:04

like naked use