Fork me on GitHub
#cljs-dev
<
2018-03-09
>
kommen09:03:34

@dnolen attached a demo project which reproduces the aot issue https://dev.clojure.org/jira/browse/CLJS-2639

dnolen11:03:11

@kommen fantastic! thanks

jannis12:03:22

Which different :package-json-resolution modes do we want to support?

:webpack => ["module", "main"]
:nodejs => ["main"]
What about "browser"?

jannis12:03:37

Should we prepend that if the :target is :browser?

jannis12:03:59

I guess that’s good.

jannis12:03:45

This is what I have now:

(package-json-entries {:target :nodejs}) => (module main)
(package-json-entries {:target :browser}) => (browser module main)
(package-json-entries {:target :nodejs :package-json-resolution :webpack}) => (module main)
(package-json-entries {:target :nodejs :package-json-resolution :nodejs}) => (main)
(package-json-entries {:target :browser :package-json-resolution :webpack}) => (browser module main)
(package-json-entries {:target :browser :package-json-resolution :nodejs}) => (browser main)

jannis12:03:59

(The default/fallback being :package-json-resolution :webpack)

dnolen12:03:27

@jannis I thought :webpack was already ["browser", "module", "main"]?

jannis12:03:55

@dnolen Well, :webpack doesn’t currently exist. We use ["module", "main"] currently if the target is :nodejs and prepend "browser" if the target is :browser.

dnolen13:03:46

@jannis right but browser is webpack

dnolen13:03:48

I don’t think we need a new thing

dnolen13:03:33

this patch should be narrow in scope

dnolen13:03:09

I do not want to add a new target, just the flag for controlling resolution

jannis13:03:50

@dnolen Ok, adding a target wasn’t my plan. So this?

:package-json-resolution :browser + any target => (browser main)
:package-json-resolution :nodejs + any target => (main)
no flag set + target :browser => (browser module main)
no flag set + target :nodejs => (module main)

jannis13:03:49

So unless the flag is set, the target determines the entries; if it is set, the flag determines the entries?

jannis13:03:41

We may not be aligned on what the supported modes should be and what entries they should result in.

bhauman13:03:41

just an idea here, but is it possible to exclude :preloads from the AOT cache?

bhauman13:03:52

or rather force their compile? They often have macros that rely on the environment. @darwin @dnolen

mfikes13:03:41

Updated the main news post to include the lists of sections and @juhoteperi’s content. https://github.com/mfikes/clojurescript-site/blob/master/content/news/2018-03-12-release.adoc @tmulvaney Did you have comments regarding the Map Entries section? (I had previously forced pushed, and perhaps GitHub made your comments inaccessible).

mfikes14:03:17

Oh, I forget @richiardiandrea were you planning on a separate news post for pREPL (and perhaps socket REPL)? If so, perhaps the section in the main news post can have a short paragraph and a link to it. ^

richiardiandrea15:03:43

You can go ahead Mike, I think my post will take time

mfikes14:03:11

Oh, I suppose somewhere we can mention Java 9 support. Maybe in the small paragraph that goes between the main header and the first section. Will add that to the draft.

jannis14:03:20

@dnolen Alright, I have the flag implemented so that, if set, it determines entries and if not, the target does. And with a flag whose entries don’t include "module" both iterall and graphql work! 🙂

jannis14:03:49

But we should decide on supported flag values and what entries they result in together. @anmonteiro Any suggestions?

tmulvaney14:03:21

@mfikes I don't think so. I think the comment I made was perhaps around a typo?

tmulvaney14:03:07

Although I guess mentioning that if a user sees errors like no such method -key found for ... that they may have been using a vector instead of a map and should adjust code appropriately.

mfikes14:03:06

@tmulvaney Yes, that was the intent of this paragraph, but perhaps it could be made more explicit

While this certainly cleans things up, be on the lookout for code that incorrectly treats vectors as map entries. For example, while (key (first {:a 1})) is perfectly valid, (key [:a 1]) is incorrect and will result in a runtime exception.

tmulvaney14:03:09

That sounds clear and succinct to me

bhauman14:03:40

@mfikes just letting you know that "re" my comment above about AOT caching. That AOT cache breaks figwheel. Not just for parallel builds but for successive builds

mfikes14:03:14

Oh, maybe we need a JIRA for that as well.

bhauman14:03:57

If you run lein figwheel for a different build you get the config from the very first figwheel build run ever

mfikes14:03:11

Not bueno.

jannis14:03:49

@dnolen @anmonteiro Current state of the patch. Let me know what you think and I’ll rework and submit it based on your suggestions. https://github.com/Jannis/clojurescript/commit/43f2facd75a9c9e0dd221e24d40825a2ce6b0b59

jannis14:03:15

Could extend the :package-json-resolution modes to :browser-modules and :nodejs-modules (which would also include "module") - those would be equivalent to what we do right now. Alternative, we could flip the names around: :browser-no-modules, :nodejs-no-modules, :browser, :nodejs. This naming may make more sense in the future, where "module" is widely supported.

mfikes15:03:08

To make it easier to review the documentation suite for ClojureScript 1.10.x, I’ve put together a gist containing auto-updating links for each document (pointing at their original repo/branch), and each associated PR. https://gist.github.com/mfikes/bdbe214f03abac88ae384adb1ac26490

bhauman15:03:00

@mfikes I'm looking at the opts-cache in the light of figwheel, should I be setting this to "[build-id].fighweel.edn" or to false?

mfikes15:03:43

Do you mean :aot-cache ?

bhauman15:03:53

opts-cache

dnolen15:03:01

@bhauman that’s just doesn’t seem critical to me sorry the :preloads thing

bhauman15:03:27

well it breaks

bhauman15:03:37

for everyone using it

dnolen15:03:44

people do all kinds of bad ideas with :preloads

bhauman15:03:02

well I'm just setting :aot-cache false then for now

dnolen15:03:20

that’s why we have the flag in the first place

bhauman15:03:08

well but most folks who use figwheel are going to have a really rough ride

dnolen15:03:18

@bhauman then provide a patch 🙂

dnolen15:03:29

I’m not going to work on it is all I’m saying and it’s not going to be a blocker

dnolen15:03:43

cljs.closure/compile-from-jar

dnolen15:03:48

exclude :preloads nses

dnolen15:03:35

@jannis brevity is preferred

jannis15:03:55

@dnolen Meaning? 😉

dnolen15:03:06

no long names for the values

dnolen15:03:48

@juhoteperi https://dev.clojure.org/jira/browse/CLJS-2633 any initial thoughts on this, broke w/ your Closure fixes

richiardiandrea15:03:14

:preloads and non preloads? What if this one is a classic :aot-cache-exclude? Just brainstorming. @bhauman

richiardiandrea15:03:28

Now that you raised the problem @bhauman I think @juhoteperi does similar things in boot-cljs so pinging him 😃

jannis15:03:40

@dnolen Ok. It was more about what kind of values/modes we want to provide and, if we were going to provide module vs. no module variants, which one we’d prioritize by giving them shorter names. I’m totally up for short names. 🙂

bhauman15:03:51

I don't have time for this today sooo, feel free to break the world

dnolen15:03:14

@bhauman I’m also confused why we rely on macros for environment config when you can usually do this via goog requires? I think environment config via macros should be considered a anti-pattern

dnolen15:03:45

Figwheel & devtools need to stop doing stuff that obviously create caching problems and hacks

dnolen15:03:27

@bhauman we’re not releasing today anyway, and likely not early next week either 🙂

dnolen15:03:38

the backlog is growing now that people are actually trying this

dnolen15:03:13

all we want is :webpack and :node

dnolen15:03:16

nothing else

dnolen15:03:30

this solves your immediate problem, and then we wait to see if we need to make the knob fancier

jannis15:03:03

Ok, I’m happy with that. Can I still ask what entries :webpack and :node should each map to? (I’ve tried clarifying this a couple times today.)

dnolen15:03:18

@jannis yes I answered that 🙂

dnolen15:03:24

:webpack is what we are doing now

dnolen15:03:26

leave it alone

dnolen15:03:38

:node is just whatever Node.js does

jannis16:03:38

Ok. :webpack does ["module", "main"] if target is :nodejs and ["browser", "module", "main"] if target is :browser. :nodejs would do ["main"] if target is :nodejs and ["browser", "main] if target is :browser. Correct?

dnolen16:03:16

I do not want this conflation of target and the resolution order

jannis16:03:19

I get that, but the current resolution order depends on the target already.

jannis16:03:20

And if you want to target the browser but don’t want "module" to be used, you’d still want ["browser", "main"]. And that doesn’t work if we make :nodejs just => ["main"].

dnolen16:03:16

Probably the sensible thing to do is simplify this and allow a user supplied value

mfikes16:03:34

@bhauman I’ve never used :opts-cache; don’t know how it behaves

jannis16:03:50

Sure, that would work too. :package-json-resolution ["browser", "main"] then?

dnolen16:03:57

:webpack is ["browser", "module", "main"], :node is ["main"]

dnolen16:03:04

and you can supply the vector explicitly

jannis16:03:36

Ah, so we’d support :webpack, :nodejs or a vector as values?

jannis16:03:39

That would be cool.

dnolen16:03:06

if no :target, :webpack implicit

dnolen16:03:20

if :nodejs, :nodejs implicit

dnolen16:03:25

that’s all we should do

dnolen16:03:48

and note this in release notes that this is a change, but easy for user to supply old behavior

jannis16:03:22

Ok. Thanks - and sorry this required such a long discussion. 😉

jannis16:03:33

I’ll rework my patch to this and then submit it to the JIRA issue.

dnolen16:03:02

@bhauman @mfikes .opts-cache is for cljs.main so REPLs started on that directory don’t blow everything away w/ different compiler options

dnolen16:03:23

@darwin it would be nice to understand why devtools needs macros to do anything

darwin16:03:32

@dnolen it is just another way how to configure it via environmental variables, beside :external-config in compiler options or user-specified config

dnolen16:03:49

@darwin but I’m asking if you need it, which I think the answer is no

dnolen16:03:57

you could do this via goog-define

dnolen16:03:37

goog-define is a runtime thing so you don’t encounter these caching issues

darwin16:03:06

maybe you are right and this was a bad idea from the beginning

dnolen16:03:07

that we can’t compile something from a JAR and know it’s reusable is not good

dnolen16:03:35

and excluding preloads is not the answer

dnolen16:03:42

we want to cache all this tooling stuff

dnolen16:03:59

waiting for devtools and figwheel to compile is not particularly awesome

mfikes16:03:24

Expanding on this, anyone could ship a JAR with macros that consult the ambient environment during compilation and thus produce “mutable” compilation artifacts. 😞

dnolen16:03:35

@bhauman ^ excluding preloads is really not something that we should do - I think tooling should sync on being aot-cacheable

dnolen16:03:53

@mfikes right so people doing that need to stop … like now

dnolen16:03:02

so going out with this default is the right thing to do

dnolen16:03:05

so people fix this stuff

john16:03:07

Which projects might be most vulnerable to this? One's that require cljs.env? Might make sense to test against a few of those.

dnolen16:03:52

any project which uses macros to generate code where the environment can change between builds

dnolen16:03:01

so excluding :preloads isn’t going solve the actual problem anyway

dnolen16:03:53

really the only answer is that type of config must be pushed into the runtime

john16:03:38

I've got some free time today. If a solution is proposed, I can help test it against some projects in the wild.

richiardiandrea16:03:11

The cljs.env thing (for completions for instance) is still outstanding and I think it is implemented as macro in many places

dnolen16:03:30

cljs.env is only a problem for libraries that use it

dnolen16:03:32

not directly for app code

dnolen16:03:03

but if a app is broken across many deps same issue

dnolen16:03:07

yeah I just don’t think there is an answer to this problem - we could just restrict :aot-cache to cljs.main

dnolen16:03:30

so at least beginners and people doing quick stuff get it

dnolen16:03:10

and of course people who are more careful about their deps can turn it on

richiardiandrea16:03:18

What if instead of blacklist we whitelist? So if tool maintainers want they can aot cache things @dnolen

john16:03:23

Could :aot-cache be...

john16:03:31

was going to ask the same thing

dnolen16:03:36

no whitelist/blacklist, this is all way too complicated

dnolen16:03:40

we’re just turning it off

dnolen16:03:46

except for cljs.main

john16:03:05

Can :aot-cache be enabled per dep?

dnolen16:03:55

that’s the same idea

dnolen16:03:21

I don’t think adding these kinds of controls are a good idea

dnolen16:03:39

beginners get it, and experts get it

dnolen16:03:51

everybody in the middle needs to fix their deps or get maintainers to fix

john16:03:12

agh, the glory of global caching... was so close 😂

richiardiandrea16:03:15

FWIW while we were speaking I explored piggieback again and apart from many set! and nastiness there is no macro there because it looks like it is passing a JavascriptEnv wrapper: https://github.com/cemerick/piggieback/blob/master/src/cemerick/piggieback.clj#L220

dnolen16:03:43

@darwin @bhauman disabling :aot-cache since the repercussion is way beyond devtools & Figwheel but I still think the macro stuff needs to go - it would be nice for people are starting out to include Figwheel & devtools and not care about this stuff - doing this via :preloads isn’t going to happen

darwin16:03:15

@dnolen I’m fine with removing that functionality from my libraries, I will write a detection and warn anyone still using it (when I detect env variables with relevant prefixes)

darwin16:03:11

thinking about it more I would maybe suggest :aot-cache debug option, that would work as :aot-cache false but warn on any cache mismatch, this way it could warn people about specific files which are causing troubles, not sure how hard this would be to implement, but it would be friendly people using potentially side-effecting code after upgrading cljs compiler

dnolen16:03:49

@darwin I don’t see anyway to implement that

dnolen16:03:15

these are macros run for side-effects, we cannot know about such things

dnolen16:03:13

so making the world :aot-cache safe will be a community effort / education project

darwin16:03:18

I mean if a macro is called twice and it emits different code each time, it would generate different AOT hash or something the cache is using

dnolen16:03:36

if something is cached we never look at it again

dnolen16:03:43

there’s no chance to observe the results of the macro

dnolen16:03:56

anyways, not going to spend anymore time on this 🙂

dnolen16:03:07

it’s just going to require a lot of work

dnolen16:03:13

report issues on libraries you like that do this

richiardiandrea16:03:21

Yeah the above would be like compiling not caching I guess

dnolen16:03:00

any library that relies on macros for config, should instead rely on goog-require

dnolen16:03:13

you can embed strings, those can be EDN so this is not really a problem for tooling

dnolen16:03:06

gotta run, but will check in later

darwin16:03:17

I had a bad experience with goog-require and :advanced mode compilation, that is why I switched to macros reading env and recommended this to others, but I will revisit it

darwin16:03:37

I’m fine with :external-config in compiler opts, that is fine solution

darwin16:03:03

I assume changing it invalidates the caches

dnolen16:03:09

:external-config won’t work

dnolen16:03:16

you don’t want to invalidate the world

dnolen16:03:21

what I’ve said above holds

dnolen16:03:25

stop using macros

dnolen16:03:32

no other changes are going to made around this

dnolen16:03:53

goog-require doesn’t need to part of the caching computation because it’s runtime

dnolen17:03:12

@mfikes should definitely mention why we can’t enable in our post and point out tooling and libs need to switch to goog-define

darwin17:03:27

losing :external-config functionality is worse than losing ability to rely on env vars - should compiler warn on any usage of :external-config then? or should this be responsibility of individual libs that relied on it?

darwin17:03:22

a wild idea: compiler could serialise :external-config into a string (edn) and then publish it via goog-define in some synthesized cljs namespace (e.g. cljs-config.external or something like that) - this way any lib interested in those values could get them at runtime (after parsing the string or there could be some support for parsing it and storing as json cljs map for easier consumption)

dnolen17:03:32

Hrm I don’t know what you’re taking about

dnolen17:03:08

Oh sorry missed second part

dnolen17:03:13

That possible

dnolen17:03:58

But that’s a convenience, libs can solve anyway

darwin17:03:04

yes, I agree that would be a convenience, goog-define supports only strings, numbers and booleans, also people have to be aware of munging for advanced mode

darwin17:03:29

I will think about it more, I can probably do that all on per-library basis and let people config stuff via one goog-define per library, they will have to specify the config as a json or edn string

anmonteiro17:03:23

@jannis

{:target <any>
     :package-json-resolution :browser} => ("browser", "main")

anmonteiro17:03:27

looks wrong to me

anmonteiro17:03:33

where’s module?

anmonteiro17:03:46

:package-json-resolution :browser that doesn’t include "module" still leads to the problems we talked about yesterday

bhauman17:03:30

glad to hear that :aot-cache is disabled for now. I agree that artifacts should not change based on the environment.I, too, struggled with various config paths before I settled on preloads. Things have changed and

bhauman17:03:46

Not sure what method is intended by goog-require

bhauman17:03:10

yeah I get goog-define

jannis18:03:29

@anmonteiro Latest version of the patch (this is what I was planning on submitting to JIRA): https://github.com/Jannis/clojurescript/commit/9e27d15029b7e62998c6784e1e8592fbc2c71b7b

jannis18:03:50

(Which has "module" unless you choose :package-json-resolution :nodejs or provide a custom vector/list of entries altogether.)

anmonteiro18:03:45

@jannis looks good to me, thanks for adding tests

jannis18:03:51

Of course 🙂

mfikes19:03:40

Documentation related to shared AOT cache has been updated: https://gist.github.com/mfikes/bdbe214f03abac88ae384adb1ac26490

darwin19:03:36

where should I expect :closure-defines to be emitted in generated js? I did a quick stab at the idea of generating runtime config from :external-config, but the resulting out dir does not seem to have :closure-defines generated anywhere

darwin20:03:52

I have to give up for now, if anyone sees why :closure-defines are not really reflected in generated code from test-runtime-config, I would be grateful: https://github.com/darwin/clojurescript/commit/486bc8d97331a6a3daa19c71120e5b070eb3fa4c

dnolen20:03:50

@darwin CLOSURE_UNCOMPILED_DEFINES

dnolen20:03:54

all entry point files declare this before loading anything

darwin20:03:59

btw. unrelated note docs says “When :optimizations :none, closure-defines have no effect unless :main is specified.“, , this seems to look like a deal breaker

dnolen20:03:55

@darwin I don’t see why, :main is almost always defined

dnolen20:03:09

since you need this for other compilation modes if you don’t want random stuff in your build

darwin20:03:27

ok, I see, but I cannot really force people to use :main if they want to configure my lib, also it will unexpectedly break (stop seeing closure-defines) when they switch from :main to old-way and back

dnolen20:03:45

@darwin I don’t think you understand

dnolen20:03:58

if you don’t use :main then that means you’re building your HTML by hand

dnolen20:03:05

and you’re writing all your script tags in order

dnolen20:03:27

like how people did years ago before :main was a thing

dnolen20:03:32

and they always got it wrong

dnolen20:03:14

I don’t think many people are doing this anymore, so I wouldn’t be concerned about it

darwin20:03:02

ok, I can live with that, and maybe warn during compilation if I see :closure-defines related to my library and no :main or :whitespace mode

darwin20:03:58

ah, forget this, this would again need a macro, which is wrong

dnolen20:03:38

@darwin really the people not using :main was must be quite small

dnolen20:03:46

nobody wants to remember how to write this markup

dnolen20:03:17

you must manually require goog.base, the deps, then cljs deps, and then the entry point you want and then invoke goog.require

dnolen20:03:21

there’s no way people are doing this anymore

dnolen20:03:10

anyways, I think relying on goog-define at this point is safe

darwin20:03:28

I see, I was there, the problem I’m concerned is that I won’t remember this and when someone tells me that his project doesn’t respect config options we could burn time on hunting this down

darwin20:03:45

they unintentionally switch to :whitespace, and then wonder

dnolen20:03:58

this person would be ClojureScript user from 2014 using an old version of ClojureScript

dnolen20:03:17

which isn’t going to work anyway

dnolen20:03:48

so I think it’s either theoretical or a too small a population to care that much

darwin20:03:55

ok, thanks for your explanations, I think I can live with this risk 🙂

dnolen20:03:59

and those people can get the answer on the usual channels

darwin20:03:36

btw. in that test case above I was missing :output-to, so I didn’t see generated CLOSURE_UNCOMPILED_DEFINES

dnolen20:03:14

right everyone is supplying :main and :output-to these days - I don’t know the last time I saw a project or repro case that didn’t involve them

dnolen20:03:21

this practice evaporated with the first versions of the Quick Start

dnolen20:03:36

pretty sure that covered :main as alternative to remembering all the gunk

darwin20:03:34

well the problem was that I copy and pasted the thing from a different test case and tweaked it a bit, I don’t really remember compiler options and their inter-dependencies, usually I copy and paste my cljsbuild configs around, where I don’t really think about it much 🙂

darwin20:03:07

all is good now, I will play with it a little bit to get some working PoC

dnolen20:03:15

@jannis only issue is that we don’t have :browser target, that’s just the default, so I don’t think we want to make it a thing now

jannis22:03:44

Ah! Ok, I can remove that part from the patch, no problem. Does everything else look ok?

dnolen20:03:08

@darwin we could start warning about dropping :main (in build) to discourage the old way of doing things

dnolen20:03:31

it’s probably best to keep the load order of the various files we generate an implementation detail

mfikes20:03:15

@dnolen FWIW, I did some research into https://dev.clojure.org/jira/browse/CLJS-2641 and found out what it is doing and left a note to that effect, without yet coming up with a solution.

darwin20:03:19

good idea, I would also warn on usage of :closure-defines when it has no effect, e.g. with :whitespace mode

darwin20:03:45

and also warn on usage of :main when there is no :output-to 🙂

dnolen20:03:02

@darwin hrm is that true re :whitespace?

darwin20:03:30

see the details at the end

darwin20:03:12

what about :simple mode?

dnolen21:03:36

@darwin works with :simple

dnolen21:03:46

and I’m skeptical that it doesn’t work with :whitespace

darwin21:03:07

it there a way to use build api in clojurescript test suite and then also run resulting js file in some js interpreter? I don’t really see existing code which would be doing that

darwin21:03:40

the main cljs test suite is compiled with single compiler options…

darwin21:03:18

I would write some tests where I would compile :closure-defines with different combinations of compiler options and then run resulting js to see if it had effect

darwin21:03:52

this would probably require dependency on headless chrome, phantomjs or something similar

dnolen21:03:55

only raw JS engines

dnolen21:03:06

but we do a lot of build tests that just check output of compiled files

dnolen21:03:11

and don’t run

darwin21:03:02

yeah, that is what I’m going to do for now, but it is not ideal

darwin21:03:16

but it is fast at least

mfikes21:03:10

Interestingly script/test-cli is set up where you could write variations passing in different compiler options via -co and have it run the results. But it is really meant to test cljs.cli and cljs.main

dnolen21:03:08

@mfikes oh right Nashorn?

mfikes21:03:58

Yeah, it defaults to Nashorn, but can use Node or whatever is passed as the first argument to the script https://github.com/clojure/clojurescript/blob/master/src/test/cljs_cli/cljs_cli/test_runner.clj#L8

mfikes21:03:16

CI runs it with nashorn, node, and rhino

dnolen21:03:30

forgot about these

mfikes21:03:33

It also asserts on the printed output of the script

darwin21:03:01

this is what I need

mfikes21:03:56

You could imagine adding -co to a test and asserting that the :closure-defines make it all the way through to the printed output of the script

mfikes21:03:51

@darwin I also forgot, if you actually need to make an assertion about the generated JavaScript, in lieu of asserting on the behavior what happens when you run it, you can use with-post-condition and write any function that is passed :output-dir value as an argument

jannis22:03:44

Ah! Ok, I can remove that part from the patch, no problem. Does everything else look ok?

bhauman22:03:17

Looks like I found a bug, with pulling in libs and perhaps foreign deps, the compiler is choking on emacs temp files, in cljs.js-deps/library-graph-nodes

bhauman22:03:35

these dang temp files

bhauman22:03:07

starts at build-index

bhauman22:03:34

Gives a file not found exception because the emacs temp file .#something.js is a link and not a file

bhauman22:03:00

JIRAting it now