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

@kommen fantastic! thanks

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

:webpack => ["module", "main"]
:nodejs => ["main"]

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

I guess that’s good.

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)

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

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

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

@jannis right but browser is webpack

I don’t think we need a new thing

this patch should be narrow in scope

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

Git bisect now available for https://dev.clojure.org/jira/browse/CLJS-2641

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

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

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

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

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

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

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

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.

@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! 🙂

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

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

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.

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

That sounds clear and succinct to me

@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

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

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

Not bueno.

@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

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.

https://dev.clojure.org/jira/browse/CLJS-2642

@dnolen @darwin ^

Perhaps as simple as a filter in this code https://github.com/clojure/clojurescript/blob/b25d5203d5114b25a0590c9d5b1d7bad86353d19/src/main/clojure/cljs/analyzer.cljc#L3857

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

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

Do you mean :aot-cache ?

opts-cache

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

well it breaks

for everyone using it

people do all kinds of bad ideas with :preloads

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

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

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

@bhauman then provide a patch 🙂

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

cljs.closure/compile-from-jar

exclude :preloads nses

@jannis brevity is preferred

@dnolen Meaning? 😉

no long names for the values

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

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

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

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

@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

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

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

the backlog is growing now that people are actually trying this

@jannis less scope

all we want is :webpack and :node

nothing else

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

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

@jannis yes I answered that 🙂

:webpack is what we are doing now

leave it alone

:node is just whatever Node.js does

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?

no

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

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

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

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

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

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

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

and you can supply the vector explicitly

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

That would be cool.

yes

if no :target, :webpack implicit

if :nodejs, :nodejs implicit

that’s all we should do

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

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

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

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

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

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

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

you could do this via goog-define

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

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

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

and excluding preloads is not the answer

we want to cache all this tooling stuff

waiting for devtools and figwheel to compile is not particularly awesome

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

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

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

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

so people fix this stuff

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

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

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

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

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

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

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

not directly for app code

but if a app is broken across many deps same issue

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

so at least beginners and people doing quick stuff get it

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

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

Could :aot-cache be...

was going to ask the same thing

no whitelist/blacklist, this is all way too complicated

we’re just turning it off

except for cljs.main

Can :aot-cache be enabled per dep?

that’s the same idea

no

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

beginners get it, and experts get it

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

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

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

@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

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

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

:aot-cache is disabled in master, enabled for cljs.main https://github.com/clojure/clojurescript/commit/92cbedd12cd39d6bc3083a269a10fba9daa7fc40

@darwin I don’t see anyway to implement that

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

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

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

if something is cached we never look at it again

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

anyways, not going to spend anymore time on this 🙂

it’s just going to require a lot of work

report issues on libraries you like that do this

Yeah the above would be like compiling not caching I guess

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

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

gotta run, but will check in later

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

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

I assume changing it invalidates the caches

:external-config won’t work

you don’t want to invalidate the world

what I’ve said above holds

stop using macros

no other changes are going to made around this

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

bbiab

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

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?

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)

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

Oh sorry missed second part

That possible

But that’s a convenience, libs can solve anyway

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

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

@jannis

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

looks wrong to me

where’s module?

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

UMD etc

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

Not sure what method is intended by goog-require

goog-define: https://cljs.github.io/api/compiler-options/closure-defines

yeah I get goog-define

cool

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

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

@jannis looks good to me, thanks for adding tests

Of course 🙂

Patch submitted to https://dev.clojure.org/jira/browse/CLJS-2592

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

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

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

@darwin CLOSURE_UNCOMPILED_DEFINES

all entry point files declare this before loading anything

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

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

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

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

@darwin I don’t think you understand

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

and you’re writing all your script tags in order

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

and they always got it wrong

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

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

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

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

nobody wants to remember how to write this markup

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

there’s no way people are doing this anymore

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

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

they unintentionally switch to :whitespace, and then wonder

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

which isn’t going to work anyway

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

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

and those people can get the answer on the usual channels

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

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

this practice evaporated with the first versions of the Quick Start

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

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 🙂

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

@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

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

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

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

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

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

@darwin hrm is that true re :whitespace?

googled it here: https://cljs.github.io/api/compiler-options/closure-defines

see the details at the end

what about :simple mode?

@darwin works with :simple

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

Our own page reflects that as well https://clojurescript.org/reference/compiler-options#closure-defines

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

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

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

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

yeah, no

only raw JS engines

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

and don’t run

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

but it is fast at least

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

@mfikes oh right Nashorn?

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

CI runs it with nashorn, node, and rhino

@darwin ^

forgot about these

It also asserts on the printed output of the script

this is what I need

Take a look at these tests @darwin https://github.com/clojure/clojurescript/blob/master/src/test/cljs_cli/cljs_cli/test.clj

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

Here is an example defining the source of the script to run https://github.com/clojure/clojurescript/blob/master/src/test/cljs_cli/cljs_cli/test.clj#L13

thanks

@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

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

these dang temp files

starts at build-index

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

JIRAting it now