Fork me on GitHub
#cljs-dev
<
2018-01-10
>
thheller09:01:30

@juhoteperi looking over your closure update it looks like you ALWAYS access module$* via ["default"]? Is that not incorrect if the source is actually ES6 and not CJS?

juhoteperi09:01:36

@thheller at least if ES6 doesn't export default

thheller09:01:42

say the ES6 does export { foo }. (:require ["that-es6" :refer (foo)]). would that not be incorrectly rewritten as module$that_es6["default"].foo?

juhoteperi09:01:19

This changes aim would be to get Closure working like before, so I guess it should only emit ["default"] for CJS and not for ES6 at all?

juhoteperi09:01:25

Then ES6 default can be implemented later

thheller09:01:56

my guess is that it now fixes CJS but breaks ES6 but I might be wrong. only looked over your patch, did not test it yet

thheller09:01:26

ES6 on npm is rare but it does exist

juhoteperi09:01:52

Problem with checking the module-type is same as with checking :js-module-index; the var name at emit* is the full module$ name and it is inconvenient to find information using that

juhoteperi09:01:37

Maybe... form has the used namespace/name from code, and maybe compiler env has the require information from alias/refer to "pretty" module-name mapping

thheller09:01:23

I still think that https://dev.clojure.org/jira/browse/CLJS-2376 is the solution we should go for. fixes every issue.

juhoteperi09:01:37

Yes, but it is separate

juhoteperi09:01:50

and should be easyish to implement once this is done properly

thheller09:01:22

it should be pretty simple to extract the necessary info from the closure compiler

juhoteperi09:01:56

But currently the harder part is to get the information from Closure to Cljs compiler emit*

juhoteperi09:01:22

Hmm, but maybe we could use that :refer default :rename {default ...} even for CJS

juhoteperi09:01:43

though that doesn't work for :as

thheller09:01:05

oh I forgot about :refer … yeah :default doesn’t fix the issue for CJS

thheller09:01:50

so basically all you are missing is (is-rewritten-commonjs? some-ns)?

juhoteperi09:01:37

no that exists, but some-ns is not available in emit*

thheller09:01:18

sure it is?

thheller09:01:06

should be either (name n) or (namespace n)? eg. js/module$foo$bar or module$foo$bar/thing?

juhoteperi10:01:30

hmm, currently we don't have any mapping from module$... to any information

juhoteperi10:01:17

:js-module-index has e.g. lodash/array -> module$path$lodash$array mapping

juhoteperi10:01:42

I guess we could add new mapping to compiler env with module$ name to module-type (and maybe other information)

thheller10:01:24

ah ok, I have that info in shadow-cljs available already. it is pretty useful at times

dnolen15:01:55

@juhoteperi we definitely want that, we worked around missing that several times already

juhoteperi15:01:15

I'll work on that today

juhoteperi15:01:52

Hm, we are probably missing any ES6 module tests now, I should probably add one

juhoteperi15:01:32

Do we have any tests where the compiled JS is ran on some JS engine to validate it works? Or is it best to just assert the output looks correct

dnolen15:01:20

@juhoteperi we don’t have a way to test that code works, though I want to fix that in this release by providing a Nashorn runner

dnolen15:01:31

once that’s in we should start using it in our tests

thheller16:01:45

and one more JS import hack, they just keep coming. 😛 https://twitter.com/_developit/status/951122721429213184

mfikes17:01:58

It looks like a directly reducible tree-seq is within reach, so long as perf benchmarks pan out. 🙂 Initial patch added for comment to: https://dev.clojure.org/jira/browse/CLJS-2464

Alex Miller (Clojure team)17:01:22

this seems bad for the same reasons as your line-seq idea

bronsa17:01:18

a bit out of the loop, why is it a bad idea?

Alex Miller (Clojure team)17:01:45

because you’re relying on seqs with potential side effects

bronsa17:01:49

right, so the same issues as iterate except iterate gets around it by requiring f to be side-effect free?

bronsa17:01:44

gotcha, thanks

mfikes17:01:28

Right, @alexmiller. So tree-seq is fundamentally a different beast than iterate?

mfikes17:01:00

I agree that if file-seq is built on tree-seq clearly there is a problem of sorts.

Alex Miller (Clojure team)17:01:02

the set of things that we made reducible in Clojure was intentionally very narrow - range, cycle, repeat all deal with values. iterate leans on the side-effect free constraint for f

mfikes17:01:11

Ahh crap. OK

Alex Miller (Clojure team)17:01:37

collections are concrete and immutable

Alex Miller (Clojure team)17:01:16

yes, but it would also allow you to do things that break expectations

Alex Miller (Clojure team)17:01:59

if you reduce over the tree-seq and then use it as a seq you are re-computing any non-concrete seq inside the tree

mfikes17:01:02

So, in short, if tree-seq’s docstring included the caveat that branch? and children “must be free of side-effects”, then this would be worth exploring. But, alas, no.

Alex Miller (Clojure team)17:01:35

yeah, it’s not something that can be applied generally to other seq functions

Alex Miller (Clojure team)17:01:49

unless you constrain other dimensions

mfikes17:01:52

Cool. Better to know now than later 🙂 Thanks!

mfikes17:01:05

In hindsight, I wish tree-seq were like iterate and not like repeatedly. Ugh.

Alex Miller (Clojure team)17:01:33

that would also limit its utility so tradeoffs either way

Alex Miller (Clojure team)17:01:56

seqs cache and are immutable - that is both a pro and a con

mfikes17:01:01

Thanks. I closed CLJS-2464 with an explanation capturing the above.

juhoteperi17:01:57

Should ./script/test pass on master?

juhoteperi18:01:40

Failing for me on master and r1.9.946. It should be enough to run build and bootstraap before test script?

juhoteperi18:01:14

I have now module-type read from Closure and added to new property on compiler-env (naming this property is maybe the hardest part...) and emit* is now checking that information and adding ["default"] only to CommonJS modules

juhoteperi18:01:39

Unfortunately looks like Closure will return proper module-type only for npm modules

juhoteperi18:01:06

Does anyone know ES6 npm packages I could use for testing? It seems to be quite hard to find one

juhoteperi18:01:23

found lodash-es

rarous20:01:59

material-components-web

juhoteperi18:01:48

Aha, even ES6 files from npm packages get module-type NONE

mfikes19:01:04

@juhoteperi With respect to script/test passing, I’ve been running the tests continually, and it passes in CI https://github.com/mfikes/clojurescript/commits/master

thheller19:01:22

looks correct yes

thheller19:01:18

although your switched to es5 as the default right? then I think you are supposed to emit .default instead

thheller19:01:34

just so it is consistent with what the closure compiler does

juhoteperi19:01:55

Closure uses ["default"] in the processed modules

thheller19:01:59

and you don’t run into situations where sometimes .default is used (by the GCC) and sometimes ["default"] by CLJS

thheller19:01:21

thought I saw it generate a .default before but just might be remembering that wrong

juhoteperi19:01:58

@thheller Maybe it was Cljs compiler? Or maybe they changed something.

juhoteperi19:01:40

In the :default issue you mention that :refer :rename would already work, but wouldn't default have been rewritten to default$?

juhoteperi19:01:48

Or did you use language-out es5? Then it wouldn't be renamed?

thheller19:01:47

I work arround the default$ is in other ways. also don’t use the closure compiler to process node_modules so don’t really have any of the issues you are working through currently

thheller19:01:10

but yes :default is still rewritten to default$

thheller19:01:31

(but I default to :language-out :ecmascript5 so not always

juhoteperi19:01:22

(:require ["lodash-es/takeRight" :default take-right])

(take-right #js [1 2])
module$home$juho$Source$clojurescript$node_modules$lodash_es$takeRight["default"]([(1),(2)]);

juhoteperi19:01:00

implemention change is +4/-1 lines plus error message changes

juhoteperi20:01:03

https://github.com/Deraen/clojurescript/commit/db622472bc6f8e2a6da7f72dca03af207f505bbd I guess this at least needs better name for compiler env prop that :js-module-index-2

thheller20:01:44

maybe :cljs.analyzer/js-namespaces similar to :cljs.analyzer/namespaces?

juhoteperi20:01:50

:default commit here: https://github.com/Deraen/clojurescript/commit/9b53bf5d957673281c5edc4c851c6566fecb1713 (in fact also requires require spec validation change)

juhoteperi20:01:00

ana/js-module-exists? can be rewritten to use the new compiler env prop, but I'll leave that to separate change