This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2017-07-31
Channels
- # beginners (9)
- # boot (38)
- # cider (7)
- # cljs-dev (181)
- # cljsrn (49)
- # clojure (136)
- # clojure-italy (44)
- # clojure-losangeles (1)
- # clojure-news (1)
- # clojure-russia (3)
- # clojure-sanfrancisco (1)
- # clojure-serbia (2)
- # clojure-spec (28)
- # clojure-uk (41)
- # clojure-ukraine (1)
- # clojurescript (103)
- # core-async (6)
- # core-logic (46)
- # cursive (5)
- # data-science (8)
- # datascript (6)
- # datomic (5)
- # emacs (35)
- # events (3)
- # jobs (2)
- # jobs-rus (2)
- # juxt (6)
- # lumo (7)
- # off-topic (101)
- # om (6)
- # onyx (6)
- # parinfer (38)
- # pedestal (5)
- # perun (1)
- # planck (4)
- # protorepl (4)
- # re-frame (62)
- # reagent (20)
- # remote-jobs (1)
- # ring-swagger (1)
- # spacemacs (16)
- # unrepl (43)
- # vim (13)
Ooh, mark thinks his patch on CLJS-2221 might be a better approach.
@anmonteiro I think it might be best to split the test for relative-name like this:
(deftest test-relative-name
(let [initial (System/getProperty "user.dir")]
(if (= File/separator "\\")
(do
(System/setProperty "user.dir" "C:\\Users\\user\\clojurescript")
(is (= (util/relative-name (io/file "C:\\Users\\user\\clojurescript\\out\\index.js")) "out\\index.js"))
(is (= (util/relative-name (io/as-url (io/file "/Users/user/clojurescript/out/index.js"))) "out\\index.js")))
(do
(System/setProperty "user.dir" "/Users/user/clojurescript")
(is (= (util/relative-name (io/file "/Users/user/clojurescript/out/index.js")) "out/index.js"))
(is (= (util/relative-name (io/as-url (io/file "/Users/user/clojurescript/out/index.js"))) "out/index.js"))))
(System/setProperty "user.dir" initial)))
Reason being the underlying http://java.io calls are platform specific. I can't see how to avoid needing Windows to test the behaviour.
This test at least explicitly shows what would happen in each case which is nice.
sounds good to me
Okay, I'll submit a patch for the test
@anmonteiro Okay, quick wrap up.
The patch on CLJS-2221 is my preferred way to fix the bug. It was submitted before your "\" appending code.
The patch on CLJS-2285 (specifically CLJS-2285b.patch) is an updated test for util/relative-name
Tests pass with the CLJS-2221 patch.
awesome
I don't think the "\" appending will be necessary any more.
bye for now.
@anmonteiro I can confirm that always enabling AMD modules makes :module-type
obsolete. shadow-cljs does not use it at all
@thheller Cljs also converts all JS files at once, per module-type
ah right, still had the old version in mind that called the ProcessEs6Modules thing directly
I’m fine with exploring removal of :module-type
and @thheller’s suggestions around that.
Just updated Cljs to 1.9.854 on a work project and started using :global-exports
, working fine with React + Leaflet + React-leaflet. Now I could try using npm packages, without any changes to the code.
React-leaflet seems to have some problems, probably related the known problems with ES6: https://github.com/PaulLeCam/react-leaflet/blob/master/src/Pane.js#L6 WARNING: JSC_JS_MODULE_LOAD_WARNING. Failed to load module "warning"
Is the npm indexing code reading the ES6 files instead of the compiled ES5: https://github.com/PaulLeCam/react-leaflet/blob/master/lib/Pane.js#L47 ?
@juhoteperi might want to try with master
@olivergeorge you left a comment on 2221, it’s not clear to me how big of a deal that is
@dnolen thanks I'll look now.
changing relative-name
is a real hassle, esp. since we don’t have a way to test many other libs with said change
Yeah, I'm not sure it's a problem either. Came out of code review as potentially surprising behaviour.
@juhoteperi this is the error I get when importing leaflet
via react-leaftlet
--- node_modules/leaflet/dist/leaflet-src.js:7
The define function must be called as a top-level statement.
@dnolen while I have you. I was looking at some other broken tests on Windows. Some are the result of expecting "/" in file paths when windows will have "\\". Where that's the case I can make them work using (util/to-path ["a" "b"]) instead of "a/b".
For example
diff --git a/src/test/clojure/cljs/closure_tests.clj b/src/test/clojure/cljs/closure_tests.clj
index 132c14e..652e905 100644
--- a/src/test/clojure/cljs/closure_tests.clj
+++ b/src/test/clojure/cljs/closure_tests.clj
@@ -90,11 +90,11 @@
(closure/maybe-install-node-deps! {:npm-deps {:left-pad "1.1.3"}})
(let [modules (closure/index-node-modules-dir)]
(is (true? (some (fn [module]
- (= module {:module-type :es6
- :file (.getAbsolutePath (io/file "node_modules/left-pad/index.js"))
- :provides ["left-pad/index.js"
- "left-pad/index"
- "left-pad"]}))
+ (= module {:module-type :es6}
+ :file (.getAbsolutePath (io/file (util/to-path ["node_modules" "left-pad" "index.js"])))
+ :provides [(util/to-path ["left-pad" "index.js"])
+ (util/to-path ["left-pad" "index"])
+ "left-pad"]))
modules))))
(test/delete-node-modules)
(spit (io/file "package.json") "{}")
Bit messy. Not sure if it's a good idea or not.
@olivergeorge why not io/file
?
Hmm, I'm getting strange error about konan
:
node_modules/konan/index.js:4
module.exports = function (src, {
^
SyntaxError: Unexpected token {
Hmm, probably due to old node/npm version
@dnolen I'll see if io/file fits. It'd be nice to get the Windows tests close to working. Best chance of keeping Windows from falling behind.
@olivergeorge would be nice to have this be a part of CI, I think there was some talk abou that
yeah, that' be ideal.
@dnolen Would you like me to raise a JIRA ticket to "Fix tests which fail on Windows due to platform specific file separators"
@thheller I don't get that error from leaflet-src
Though I'm not sure if Leaflet is being properly processed, the file has goog.provide but the rest of code looks unchanged
Still getting warning about warning
with master
@juhoteperi the rest of file shouldn’t be changed, remember we only rewrite JS requires / imports
@juhoteperi is warning
namespace present in node_modules
in your :output-dir
?
@olivergeorge re: tests, yes please
@dnolen Yes, warning is available in node_modules, under react-leaflet
but depending on it directly doesn't help
Hmm, warning
has separate browser file: https://github.com/BerkeleyTrue/warning/blob/master/package.json#L6
If I understand index-node-modules-dir
, it checks for main
entry in package.json, but not browser
But if there is no main entry, it should use module-name.js
or index.js
I believe the later statement is already true? I don’t know about browser
, we should wait for António to chime in I guess.
Output from index-node-modules-dir
looks correct, there is a entry which provides warning
@juhoteperi so maybe something about it that breakes Node.js resolution in Closure?
I do see that warning
is CommonJS and the importing library is ES6 - maybe there’s problems with that in Closure?
The problem might be of our own creating, as we process ES6 and CommonJS separately
So when CommonJS is processed, Closure doesn't know about ES6 files etc.
@juhoteperi you can try the patch that changes that
Doesn't fix this
I noticed index-node-modules-dir output includes both lib/ and src/ files:
{:file "/home/juho/Source/x/y/node_modules/react-leaflet/lib/Path.js", :module-type :es6, :provides ["react-leaflet/lib/Path.js" "react-leaflet/lib/Path"]}
{:file "/home/juho/Source/x/y/node_modules/react-leaflet/src/Path.js", :module-type :es6, :provides ["react-leaflet/src/Path.js" "react-leaflet/src/Path"]}
And that both are :es6 when though the lib/ file should be commonjs?
only lib/index.js provides "react-leaflet", so I think the src files don't get processed
@juhoteperi with António’s patch the idea is that :module-type
doesn’t actually matter anymore
Heh I was looking at cljs.closure at github but had old version open, so I didn't understand why module-type is always es6
but that was changed by cljs-2279
(cljs.closure/index-node-modules ["react-leaflet"])
;; contains {:file "/home/juho/Source/x/y/node_modules/warning/browser.js", :module-type :es6,
:provides ["warning" "warning/browser.js" "warning/browser"]}
So module_deps.js selects the file defined by browser
entry in package.json
But perhaps Closure doesn't understand this
I wonder if module_deps.js should ignore browser entry for now, until Closure supports it?
probably, let’s ping about António about this to see what cases he was trying to cover given it’s not going to work in Closure
I'll open a issue about this with notes
Replacing browserResolve with nodeResolve in module_deps.js seems to work
one concern I have is that libs may need you to use browser
if you’re targeting the browser - that’s unclear to me right now
Looks like Leaflet AMD (or what ever this is) wrapper doesn't work with Closure: https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.1.0/leaflet.js
"object"==typeof exports&&"undefined"!=typeof module
{typeof module$home$juho$Source$x$y$node_modules$leaflet$dist$leaflet_src==="object"&&typeof module!=="undefined"
and I think the second part is never true as module
is not defined
if you don’t process with AMD conversion it will transpile the code but that code won’t work
Hmm, I think this is supposed to be UMD module, not AMD
UMD wrapper that is
Closure handles some of these, but this is a bit different
enabling AMD support doesn't change this
yes you won’t be able to use it but the compiler will tell you instead of pretending that everything is fine
Okay, it fails: ERROR: NON_TOP_LEVEL_STATEMENT_DEFINE. The define function must be called as a top-level statement. at /home/juho/Source/x/y/node_modules/leaflet/dist/leaflet-src.js line 7 : 46
But this error is nonsense because the define function should not be called for this module
It us UMD module so it can be directly used as CommonJS, IF the wrapper is correctly converted
Did you check if there are existing Closure issues about not recognizing wrappers?
@juhoteperi but this might also be why browser
is silly
Hmm, yeah. Not sure why Leaflet npm package uses UMD file as main entry
Woah, I modified the Leaflet wrapper to match Closure tests, and it works, without optimizations anyway
I think the different is that Leaflet wrapper uses ternary operator, and Closure only detects wrapper using if statements
@juhoteperi I believe Maria did the UMD detection stuff in Closure, I think her change could probably be made to work with ternary operator?
Yes, probably
I'll investigate a bit more before opening an issue at Closure-compiler
!function (root, name, definition) {
if (typeof module != 'undefined' && module.exports) module.exports = definition()
else if (typeof define == 'function' && define.amd) define(name, definition)
else root[name] = definition()
}(this, 'bowser', function () {
Seem like module
check is inverse of what Closure supports: typeof module === 'object'
A few new test cases and fixes will probably fix loads of packages
Hrm last time I checked our support for the browser field in package.json worked
Our support works, but Closure doesn't have support
Uhm I see
@anmonteiro Closure node module resolution doesn’t take browser
module
into account
@dnolen got it, so should we disable it for now?
@juhoteperi btw index-node-modules-dir
will always be right because it indexes every file in node_modules
So hopefully that will never be the source of problems
@anmonteiro I think providing a PR to Closure for at least the internal changes would be nice
I could try that
Got plenty of Chad's PRs to look at I guess
I commented at https://github.com/google/closure-compiler/issues/2433, with quick idea of where this would be implemented
Thanks
@anmonteiro another thing that might make sense to do would be decouple ourselves from the Closure Compiler release cycle
So that we could apply our own patches
If needed
Yeah, that's what I meant sorry
Using master without being released
Like the NPE patch they have in master
Which would be rly nice to have today
Btw re:cljs-2286 I still don't agree with setting processamd to true all the time
Because of the UMD problem which can be parsed as commonjs
So in my patch I'm only setting AMD processing to true if there's some AMD module in there
Which will be rare anyway
Sounds reasonable to me
The errors from AMD conversion don't make any sense with UMD files
And we have tests that show that too
In cljs.module-processing-tests
@juhoteperi the tests you first added last year actually
core-js has problems as it uses var meta = module.exports = { ... }
, hmh, perhaps this was fixed by the recent change in Closure
Nope, still broken: https://github.com/google/closure-compiler/issues/2585
Oops, by Closure build was broken and seems like this was fixed by the latest change
just a thought on the package.json
browser
issue. it is JSON so reading it, (assoc json "main" (get json "browser"))
and then passing it to closure is not a big deal
Hmm, yeah, we already write the files to disk
Ah no, Closure is passed SourceFile which can be constructed from code (as string), File or InputStream
The files are written to disk only after Closure processing
I guess in node modules case the files are always at disk, under node_modules/, but it probably won't be good idea to go modifying those files in-place
... right 😄
you construct the sourcefile instances before passing them to closure, so do the rewrite there. no need to touch files
I've made a start patching tests to work in Windows. I'd appreciate a 2 min code review to decide if the (str (io/file ...)) pattern is fine before I go much further.
The ticket has a patch for one file https://dev.clojure.org/jira/browse/CLJS-2288
There are what look like a couple of bugs. I've noted them with FIXME comments in that patch for now.