This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2017-08-01
Channels
- # beginners (41)
- # boot (25)
- # cider (34)
- # cljs-dev (221)
- # cljsrn (1)
- # clojure (191)
- # clojure-dusseldorf (4)
- # clojure-hamburg (1)
- # clojure-italy (8)
- # clojure-poland (1)
- # clojure-russia (22)
- # clojure-spec (27)
- # clojure-uk (91)
- # clojurescript (101)
- # core-async (11)
- # cursive (33)
- # data-science (9)
- # datascript (3)
- # datomic (30)
- # emacs (4)
- # events (4)
- # garden (6)
- # jobs (3)
- # leiningen (8)
- # luminus (39)
- # lumo (2)
- # off-topic (158)
- # om (13)
- # onyx (1)
- # parinfer (22)
- # planck (2)
- # protorepl (5)
- # re-frame (7)
- # reagent (10)
- # remote-jobs (1)
- # ring (1)
- # ring-swagger (20)
- # unrepl (92)
- # vim (11)
@dnolen re CLJS-1620 I tried to use language-out first but ran into failing tests where we declare variables named default
That's even the case in the CLJS codebase
They won't be munged if language-out is ES6 which results in errors
It sounds like a better idea to use language-out, so should we fix those call sites?
@olivergeorge Does (str (io/file ".../..."))
convert /
-> \
?
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
In node e.g. require("react-dom/server.js")
is valid in Windows
@juhoteperi I think we take care of that
I definitely wrote code to convert \
to /
in node modules
I'm commenting on @olivergeorge's patch, which might change the test cases, if I understand it correctly
I didn't write test cases for Windows though
I think the "normal tests" are broken on windows
but I doubt changing the test cases is correct solution
And relative-name
on windows always returns backslashes for paths
I think index-node-modules-dir
should always use /
in :provides
@anmonteiro yes let’s fix
Agreed, doesn't it?
I think not, it uses getAbsolutePath
to get the full path and then selects the relative path from that
You're right
But I could swear we convert them later or something
I'm quite certain I tested "react-dom/server"
on Windows
I can give it another try today
Tests check index-node-modules-dir
output directly, so maybe they are converted later?
@dnolen sounds like a better idea. I'll amend the patch later
@juhoteperi those tests may be failing on windows
Yes, that's what CLJS-2288 is about 😄
Sorry, didn't have that context
@juhoteperi I remember now. We do that in module_deps.js
but not in index-node-modules-dir
Feel free to open a ticket and I'll fox today
That's why I could test react-dom/server
in the browser
Prpbably broken with target node
We should definitely set up the windows CI. I'll open an issue so I don't forget
I set up appveyor for Lumo which has a rather complex pipeline so should be able to do the same for ClojureScript
If somebody wants to get to it first: https://dev.clojure.org/jira/browse/CLJS-2291
@juhoteperi started looking into the Closure browser field issue yesterday
Doesn't seem too hard. I got myself comfortable with writing & running tests
Will see if I can get to it some more today
^ @olivergeorge I don't have access to Windows but I think that should fix the :provides
tests on Windows
@juhoteperi just tried Closure Compiler master and it breaks our module processing tests
@anmonteiro Yeah tested quickly yesterday and broke everything in a project, didn't check tests
or I should say, it’s breaking script/test
it’s keeping the require
s in lodash for some reason
Closure-compiler should have a few labels for issues about commonjs/es6 processing, node require resolving etc.
There doesn't seem to be many changes to ES6RewriteModules or ProcessCommonJSModules after the latest release
I was looking at this https://github.com/google/closure-compiler/compare/v20170626...HEAD
still trying to figure it out
The diff is quite large, might be easier to just bisect it
I’m bisecting
halfway
my hunch is that https://github.com/google/closure-compiler/commit/8fc82299c45b0a33f19834222acda4850fd77bc0 broke it
that’s the only revision that touched these files in the current bisect range
2 more steps to confirm
What was the error with lodash? Is it some specific lodash module?
require
is not defined
just looks like some module is not being processed
Okay, so it doesn't correctly rewrite require
in some cases
What files cause the problem?
there are a lot of files, I didn’t look into yet
let me find a bad revision again and I’ll look
actually this might be it too: https://github.com/google/closure-compiler/commit/c36fe0f6b656f3cda6732f7be50b8d6197bb1722
dunno
2 steps to go
@juhoteperi so this file https://github.com/lodash/lodash/blob/master/.internal/baseGetTag.js
somehow ends up calling require("./_objectToString")
and it has the var
thing
hmm, maybe it converts requires in two first multiple var declarations
~/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");
there
you reproed it 🙂
the test case in previous issue only had two vars 😄
oh lol
do you want to open issue at closure about this?
so they just made it work for 2 vars?
@juhoteperi I think you have more context. can you open the issue and I’ll make a comment about bisecting?
thanks
think we may have to skip this closure version
thanks
Any idea when they will do the next release?
It is possible they can fix this problem before release
The problem is probably something simple, like something missing in this loop: https://github.com/google/closure-compiler/commit/8fc82299c45b0a33f19834222acda4850fd77bc0#diff-d1b3eef5af5a700763a2fce367ab27ddR1410
@juhoteperi they release rougly once every month
and the last one was June 26
@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
so we can either get it in without a test or wait a couple months
@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']
they’ve been slowly adding these passes recently. e.g.: https://github.com/google/closure-compiler/commit/53a45ec6d00cb067e30d41deb34aa2422fc03309
@anmonteiro should be testing with :es5
out here
@dnolen sorry what do you mean?
right
so the default
emission thing should work for es5 and up?
my bad!
I misunderstood the problem then
I’m going to make default
work for this case
but I’ve noticed more things seem to work, like x.var()
or x.class()
IMO we can add those later if anybody complains. thoughts?
@dnolen also, adding tests for this will make us test CLJS with language-out es5
I’m not sure if this is desirable, or if we want to keep testing it in es3
@kevin.lynagh why hello!
@dnolen new patch in CLJS-1620
@dnolen Hey! Thought I should check out this new slack business.
As you can see, I'm pretty serious about the async.
good seeing you around @kevin.lynagh 🙂
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.
@bronsa this is looking like a tools.reader bug to me? https://dev.clojure.org/jira/browse/CLJS-2261
(defmacro soda []
`(foo))
gets read as (my.ns/foo)
whereas
(defmacro cake []
`(X.))
gets read as (X.)
^ shouldn’t it be qualified?
thanks!
I’ll keep digging on my end
(.endsWith sym ".")
(let [csym (symbol (subs sym 0 (dec (count sym))))]
(symbol (.concat (name (resolve-symbol csym)) ".")))
reason is in clojure classnames don't partecipate in the namespace system like in cljs
@bronsa is that tools reader code?
gotcha
@bronsa awesome, let me know when you make a release and I’ll bump it in CLJS
@bronsa I think I’m still seeing the same
let me confirm
sorry
hmm
clojure.tools.reader> (eval (binding [resolve-symbol {'X 'foo/X}] (read-string "`(X.)")))
(foo/X.)
perhaps we’re not resolving correctly
let me confirm
@bronsa oh wait
tests pass
but I think they’re still failing on the Clojure side of things
^ let me know if that’s not clear
so self-hosted tests pass now
they were previously failing
but the JVM analyzer still fails
I wonder if this is because of the way macros are expanded?
hmm, I don't see any significant differences in how we read
`(X.)
between cljs and cljI'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
doesn’t seem like a problem in resolve-symbol
AFAICT
because it works in self-host
@bronsa what I mean is because we expand the macros in Clojure, they’re being read by Clojure, not tools.reader
or is that not true?
relevant line: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L3268
right, so this case can only be made to work with bootstrap
now I can’t decide if we should make it work with bootstrap
or if we should maintain JVM CLJS compat
makes sense to me
right
I’m inclined to make it work for bootstrap in this ticket
the other part falls a little bit out of scope IMO
if you're interest in making that work in JVM CLJS, I can help w/ reimplementing require semantics using t.r
@bronsa take away for TR is your fix works though 👍
thanks, I’ll make the bump a part of this ticket too
@bronsa yeah if we could offload this responsbility that’s nice, but it seems strange for tools.reader to deal w/ this?
I'm familiar with how require/loading works so if you need help with that I'd be happy to do it
it also sounds like an unnecessary source of bugs for little gain
@anmonteiro to me the upside is macro file having sensible resolution 🙂
right
@dnolen attached my patch to https://dev.clojure.org/jira/browse/CLJS-2261
thanks. it’s really just Mike’s tests + treader bump 🙂
good news is dotted-symbol?
and resolve-symbol
had no bugs
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
which means that resolve-symbol
in cljs will probably need to change to deal with that
altho the clojure compiler does ignore the namespace segment on dotted symbols, so it doesn't really break anything
@bronsa sounds good, I’ll make a comment in JIRA