Fork me on GitHub
#cljs-dev
<
2017-08-01
>
anmonteiro15:08:42

@dnolen re CLJS-1620 I tried to use language-out first but ran into failing tests where we declare variables named default

anmonteiro15:08:54

That's even the case in the CLJS codebase

anmonteiro15:08:47

They won't be munged if language-out is ES6 which results in errors

anmonteiro15:08:33

It sounds like a better idea to use language-out, so should we fix those call sites?

juhoteperi15:08:35

@olivergeorge Does (str (io/file ".../...")) convert / -> \?

juhoteperi15:08:08

I'm not sure what is wanted with :provides. That should match the names used in :require forms, and / is used there even for Windows

juhoteperi15:08:20

In node e.g. require("react-dom/server.js") is valid in Windows

anmonteiro15:08:58

@juhoteperi I think we take care of that

anmonteiro15:08:26

I definitely wrote code to convert \ to / in node modules

juhoteperi15:08:31

I'm commenting on @olivergeorge's patch, which might change the test cases, if I understand it correctly

anmonteiro15:08:13

I didn't write test cases for Windows though

juhoteperi15:08:37

I think the "normal tests" are broken on windows

juhoteperi15:08:50

but I doubt changing the test cases is correct solution

anmonteiro15:08:56

And relative-name on windows always returns backslashes for paths

juhoteperi15:08:28

I think index-node-modules-dir should always use / in :provides

anmonteiro15:08:03

Agreed, doesn't it?

juhoteperi15:08:56

I think not, it uses getAbsolutePath to get the full path and then selects the relative path from that

anmonteiro15:08:45

You're right

anmonteiro15:08:06

But I could swear we convert them later or something

anmonteiro15:08:33

I'm quite certain I tested "react-dom/server" on Windows

anmonteiro15:08:42

I can give it another try today

juhoteperi15:08:05

Tests check index-node-modules-dir output directly, so maybe they are converted later?

anmonteiro15:08:08

@dnolen sounds like a better idea. I'll amend the patch later

anmonteiro15:08:52

@juhoteperi those tests may be failing on windows

juhoteperi15:08:10

Yes, that's what CLJS-2288 is about 😄

anmonteiro15:08:28

Sorry, didn't have that context

anmonteiro15:08:12

@juhoteperi I remember now. We do that in module_deps.js but not in index-node-modules-dir

anmonteiro15:08:24

Feel free to open a ticket and I'll fox today

anmonteiro15:08:56

That's why I could test react-dom/server in the browser

anmonteiro15:08:10

Prpbably broken with target node

anmonteiro15:08:08

We should definitely set up the windows CI. I'll open an issue so I don't forget

anmonteiro15:08:42

I set up appveyor for Lumo which has a rather complex pipeline so should be able to do the same for ClojureScript

anmonteiro15:08:57

@juhoteperi started looking into the Closure browser field issue yesterday

anmonteiro15:08:22

Doesn't seem too hard. I got myself comfortable with writing & running tests

anmonteiro15:08:09

Will see if I can get to it some more today

juhoteperi16:08:57

^ @olivergeorge I don't have access to Windows but I think that should fix the :provides tests on Windows

anmonteiro17:08:39

@juhoteperi just tried Closure Compiler master and it breaks our module processing tests

juhoteperi17:08:19

@anmonteiro Yeah tested quickly yesterday and broke everything in a project, didn't check tests

anmonteiro17:08:44

or I should say, it’s breaking script/test

anmonteiro17:08:59

it’s keeping the requires in lodash for some reason

juhoteperi17:08:15

Closure-compiler should have a few labels for issues about commonjs/es6 processing, node require resolving etc.

juhoteperi17:08:47

There doesn't seem to be many changes to ES6RewriteModules or ProcessCommonJSModules after the latest release

anmonteiro18:08:45

still trying to figure it out

juhoteperi18:08:52

The diff is quite large, might be easier to just bisect it

anmonteiro18:08:00

I’m bisecting

anmonteiro18:08:40

that’s the only revision that touched these files in the current bisect range

anmonteiro18:08:49

2 more steps to confirm

juhoteperi18:08:05

What was the error with lodash? Is it some specific lodash module?

anmonteiro18:08:17

require is not defined

anmonteiro18:08:29

just looks like some module is not being processed

juhoteperi18:08:33

Okay, so it doesn't correctly rewrite require in some cases

juhoteperi18:08:41

What files cause the problem?

anmonteiro18:08:09

there are a lot of files, I didn’t look into yet

anmonteiro18:08:19

let me find a bad revision again and I’ll look

anmonteiro18:08:32

2 steps to go

anmonteiro18:08:18

somehow ends up calling require("./_objectToString")

anmonteiro18:08:46

and it has the var thing

juhoteperi18:08:43

hmm, maybe it converts requires in two first multiple var declarations

juhoteperi18:08:30

~/Source/closure-compiler master*
❯ cat foo.js 
var first = require('./bar'),
    second = require('./bar'),
    third = require('./bar'),
    fourth = require('./bar');

function hello(value) {
        return 'hello';
}

module.exports = hello;

~/Source/closure-compiler master*
❯ cat bar.js 
module.exports = {
        foo: 'bar'
};

~/Source/closure-compiler master*
❯ java -jar target/closure-compiler-1.0-SNAPSHOT.jar bar.js foo.js --process_common_js_modules --module_resolution node
var module$bar={foo:"bar"};var module$foo=function(a){return"hello"},first$$module$foo=module$bar,second$$module$foo=module$bar,third=require("./bar"),fourth=require("./bar");

anmonteiro18:08:04

you reproed it 🙂

juhoteperi18:08:41

the test case in previous issue only had two vars 😄

juhoteperi18:08:13

do you want to open issue at closure about this?

anmonteiro18:08:22

so they just made it work for 2 vars?

anmonteiro18:08:40

@juhoteperi I think you have more context. can you open the issue and I’ll make a comment about bisecting?

anmonteiro18:08:09

think we may have to skip this closure version

juhoteperi18:08:48

Any idea when they will do the next release?

juhoteperi18:08:39

It is possible they can fix this problem before release

anmonteiro18:08:02

@juhoteperi they release rougly once every month

anmonteiro18:08:10

and the last one was June 26

anmonteiro19:08:48

@dnolen I have CLJS-1620 at a good state but we can’t really test it very well because Closure’s ES6 emission still doesn’t apply every optimization

anmonteiro19:08:59

so we can either get it in without a test or wait a couple months

dnolen19:08:25

what do you mean by “Closure’s ES6 emission still doesn’t apply every optimization” ?

anmonteiro19:08:22

@dnolen if I set :language-out :es6 in our tests, I get a number of warnings like this one:

WARNING: Skipping pass collapseObjectLiterals                                                                                                                 factory features:  [getters, trailing comma, ES3 keywords as identifiers, setters, reserved words as properties, string continuation]                         compiler features: [getters, trailing comma, ES3 keywords as identifiers, setters, reserved words as properties, string continuation, rest parameter, template
 literal, super, computed property, new.target, octal literal, generator, binary literal, extended object literal, array pattern rest, spread expression, const declaration, destructuring, class, modules, arrow function, RegExp flag 'u', for-of loop, default parameter, let declaration, member declaration, RegExp flag 'y']

dnolen19:08:17

@anmonteiro should be testing with :es5 out here

anmonteiro19:08:08

@dnolen sorry what do you mean?

dnolen19:08:43

hardly anyone is going to set language-out to :es6

dnolen19:08:13

everyone is going to do :es5, all the optimizations are going to work for :es5

anmonteiro19:08:31

so the default emission thing should work for es5 and up?

dnolen19:08:56

the keyword rules changed with ES5 so this is fine

anmonteiro19:08:08

I misunderstood the problem then

dnolen19:08:29

people who are targeting runtimes older than ES5 won’t want this to work

dnolen19:08:32

because it won’t work

anmonteiro19:08:33

I’m going to make default work for this case

anmonteiro19:08:51

but I’ve noticed more things seem to work, like x.var() or x.class()

anmonteiro19:08:03

IMO we can add those later if anybody complains. thoughts?

anmonteiro19:08:24

@dnolen also, adding tests for this will make us test CLJS with language-out es5

anmonteiro19:08:37

I’m not sure if this is desirable, or if we want to keep testing it in es3

anmonteiro20:08:55

@dnolen new patch in CLJS-1620

dnolen20:08:30

cool thanks

Kevin Lynagh20:08:51

@dnolen Hey! Thought I should check out this new slack business.

Kevin Lynagh20:08:02

As you can see, I'm pretty serious about the async.

martinklepsch20:08:11

good seeing you around @kevin.lynagh 🙂

mfikes21:08:04

Closure takes advantage of the @const JSDoc tag in ways that look more sophisticated than the unconditional inlining ClojureScript is currently doing with ^:const. It makes you wonder if simply emitting @const (along with @type) would lead to better :advanced code gen.

mfikes21:08:38

(See pg. 42 of Closure: The Definitive Guide)

anmonteiro21:08:17

@bronsa this is looking like a tools.reader bug to me? https://dev.clojure.org/jira/browse/CLJS-2261

anmonteiro21:08:47

(defmacro soda []
  `(foo))
gets read as (my.ns/foo)

anmonteiro21:08:04

whereas

(defmacro cake []
  `(X.))
gets read as (X.)

anmonteiro21:08:12

^ shouldn’t it be qualified?

bronsa21:08:04

I'll take a look

anmonteiro21:08:31

I’ll keep digging on my end

bronsa21:08:49

(.endsWith sym ".")
               (let [csym (symbol (subs sym 0 (dec (count sym))))]
                 (symbol (.concat (name (resolve-symbol csym)) ".")))

bronsa21:08:57

I believe that name should be str

bronsa21:08:27

reason is in clojure classnames don't partecipate in the namespace system like in cljs

bronsa21:08:38

so name is fine for clj, but elides the ns in cljs

anmonteiro21:08:41

@bronsa is that tools reader code?

bronsa21:08:49

pushing a fix

anmonteiro21:08:08

@bronsa awesome, let me know when you make a release and I’ll bump it in CLJS

bronsa21:08:31

can you test master before I cut a release?

bronsa21:08:07

1.0.4-SNAPSHOT

bronsa21:08:05

just backported the fix to the cljs port too

anmonteiro21:08:00

@bronsa I think I’m still seeing the same

anmonteiro21:08:31

let me confirm

bronsa21:08:46

hmm ok, I'll start a repl :)

bronsa22:08:02

hmm

clojure.tools.reader> (eval (binding [resolve-symbol {'X 'foo/X}] (read-string "`(X.)")))
(foo/X.)

bronsa22:08:05

looks ok to me now?

anmonteiro22:08:53

perhaps we’re not resolving correctly

anmonteiro22:08:54

let me confirm

bronsa22:08:02

I'll test the cljs version too

bronsa22:08:22

we're calling resolve-symbol with X rather than X. here

anmonteiro22:08:44

but I think they’re still failing on the Clojure side of things

anmonteiro22:08:25

^ let me know if that’s not clear

bronsa22:08:30

what's failing?

anmonteiro22:08:37

so self-hosted tests pass now

anmonteiro22:08:42

they were previously failing

anmonteiro22:08:16

but the JVM analyzer still fails

anmonteiro22:08:31

I wonder if this is because of the way macros are expanded?

bronsa22:08:45

hmm, I don't see any significant differences in how we read

`(X.)
between cljs and clj

bronsa22:08:16

I'll try to play around with cljs a bit to see if it's still a reader issue or if it's a problem in resolve-symbol

anmonteiro22:08:05

doesn’t seem like a problem in resolve-symbol AFAICT

anmonteiro22:08:11

because it works in self-host

bronsa22:08:41

ah is cljs using the same impl for both self-host and CLJ?

anmonteiro22:08:57

@bronsa what I mean is because we expand the macros in Clojure, they’re being read by Clojure, not tools.reader

anmonteiro22:08:07

or is that not true?

bronsa22:08:34

depends on how the cljs analyzer requires those macro files

bronsa22:08:50

it's been quite a while since I looked at it, I couldn't say

dnolen22:08:58

read by Clojure

anmonteiro22:08:10

right, so this case can only be made to work with bootstrap

dnolen22:08:11

we only use tools.reader for CLJS/CLJC sources

bronsa22:08:18

yeah if theyr'e read by clojure then that's your problem

dnolen22:08:18

which aren’t macro files

anmonteiro22:08:08

now I can’t decide if we should make it work with bootstrap

anmonteiro22:08:24

or if we should maintain JVM CLJS compat

dnolen22:08:35

I don’t see a problem with making it simpler for bootstrap users

bronsa22:08:41

you could make it work in JVM CLJS too :)

dnolen22:08:46

cross platform people will fully-qualify

dnolen22:08:53

@bronsa’s idea is the other alternative

anmonteiro22:08:55

makes sense to me

bronsa22:08:05

would mean not loading macro namespaces through require

dnolen22:08:20

@bronsa yeah we’d have to do our own thing - I don’t really know about that

bronsa22:08:20

and reading them form by form through tools.reader + evaluating each form

dnolen22:08:44

would be annoying to make sure we replicate Clojure setup

anmonteiro22:08:46

I’m inclined to make it work for bootstrap in this ticket

anmonteiro22:08:59

the other part falls a little bit out of scope IMO

dnolen22:08:24

I’m fine w/ it working in bootstrap - it’s just a nice to have really - not essential

dnolen22:08:30

it doesn’t damage making cross-platform stuff

bronsa22:08:48

if you're interest in making that work in JVM CLJS, I can help w/ reimplementing require semantics using t.r

anmonteiro22:08:48

@bronsa take away for TR is your fix works though 👍

bronsa22:08:58

in the meantime I'll cut a TR release

dnolen22:08:03

sounds good

anmonteiro22:08:12

thanks, I’ll make the bump a part of this ticket too

dnolen22:08:34

@bronsa yeah if we could offload this responsbility that’s nice, but it seems strange for tools.reader to deal w/ this?

bronsa22:08:56

yeah I mean helping make a patch to cljs

dnolen22:08:15

ah yeah, I’d be happy to review

bronsa22:08:23

I'm familiar with how require/loading works so if you need help with that I'd be happy to do it

dnolen22:08:25

was just concerned about it being hairy

anmonteiro22:08:35

it also sounds like an unnecessary source of bugs for little gain

dnolen22:08:50

@anmonteiro to me the upside is macro file having sensible resolution 🙂

dnolen22:08:59

I’ve been wanting that forever - it’s very confusing right now

dnolen22:08:01

even for me

dnolen22:08:20

like I said, we’ll see what the patch looks like

dnolen22:08:18

thanks! will take a closer look later

anmonteiro22:08:51

thanks. it’s really just Mike’s tests + treader bump 🙂

anmonteiro22:08:20

good news is dotted-symbol? and resolve-symbol had no bugs

bronsa22:08:11

ouch, actually wait before bumping to 1.0.4

bronsa22:08:32

that causes a regression when used to read CLJ

bronsa22:08:47

right, so the only way to support both CLJ and CLJS behaviour is to have resolve-symbol accept and resolve X. to foo/X. rather than accepting just X

bronsa22:08:51

which means that resolve-symbol in cljs will probably need to change to deal with that

bronsa23:08:40

altho the clojure compiler does ignore the namespace segment on dotted symbols, so it doesn't really break anything

bronsa23:08:59

hmm, I'll sleep on it and let you know

bronsa23:08:17

in the meantime just hold fire on that ticket please

anmonteiro23:08:40

@bronsa sounds good, I’ll make a comment in JIRA