Fork me on GitHub
#cljs-dev
<
2017-07-31
>
Oliver George00:07:17

Ooh, mark thinks his patch on CLJS-2221 might be a better approach.

Oliver George01:07:57

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

Oliver George01:07:48

Reason being the underlying http://java.io calls are platform specific. I can't see how to avoid needing Windows to test the behaviour.

Oliver George01:07:02

This test at least explicitly shows what would happen in each case which is nice.

anmonteiro01:07:50

sounds good to me

Oliver George01:07:45

Okay, I'll submit a patch for the test

Oliver George01:07:36

The patch on CLJS-2221 is my preferred way to fix the bug. It was submitted before your "\" appending code.

Oliver George01:07:04

The patch on CLJS-2285 (specifically CLJS-2285b.patch) is an updated test for util/relative-name

Oliver George01:07:18

Tests pass with the CLJS-2221 patch.

Oliver George01:07:22

I don't think the "\" appending will be necessary any more.

thheller06:07:30

@anmonteiro I can confirm that always enabling AMD modules makes :module-type obsolete. shadow-cljs does not use it at all

thheller06:07:52

however Closure cannot process JSX and other things like it

thheller06:07:07

so you still need a hook to convert some things, just not things in npm

thheller06:07:02

I also compile all JS sources at once, not one by one.

juhoteperi06:07:14

@thheller Cljs also converts all JS files at once, per module-type

thheller06:07:36

ah right, still had the old version in mind that called the ProcessEs6Modules thing directly

dnolen11:07:49

I’m fine with exploring removal of :module-type and @thheller’s suggestions around that.

juhoteperi11:07:40

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.

juhoteperi11:07:50

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"

juhoteperi11:07:43

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 ?

dnolen11:07:24

@juhoteperi might want to try with master

dnolen11:07:57

@olivergeorge you left a comment on 2221, it’s not clear to me how big of a deal that is

Oliver George11:07:18

@dnolen thanks I'll look now.

dnolen11:07:37

what I mean is I don’t know if your point is a problem

dnolen11:07:58

changing relative-name is a real hassle, esp. since we don’t have a way to test many other libs with said change

Oliver George11:07:49

Yeah, I'm not sure it's a problem either. Came out of code review as potentially surprising behaviour.

thheller11:07:18

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

Oliver George11:07:54

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

Oliver George11:07:46

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") "{}")

thheller11:07:56

which comes back to the “always process AMD modules” situation

Oliver George11:07:18

Bit messy. Not sure if it's a good idea or not.

dnolen11:07:57

@olivergeorge why not io/file ?

dnolen11:07:13

happy to see tests get fixed up for Windows

juhoteperi11:07:13

Hmm, I'm getting strange error about konan: node_modules/konan/index.js:4 module.exports = function (src, { ^ SyntaxError: Unexpected token {

juhoteperi11:07:46

Hmm, probably due to old node/npm version

Oliver George11:07:57

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

dnolen11:07:33

@olivergeorge would be nice to have this be a part of CI, I think there was some talk abou that

Oliver George11:07:59

yeah, that' be ideal.

Oliver George11:07:08

@dnolen Would you like me to raise a JIRA ticket to "Fix tests which fail on Windows due to platform specific file separators"

juhoteperi11:07:40

@thheller I don't get that error from leaflet-src

juhoteperi11:07:22

Though I'm not sure if Leaflet is being properly processed, the file has goog.provide but the rest of code looks unchanged

juhoteperi11:07:41

Still getting warning about warning with master

dnolen11:07:19

@juhoteperi the rest of file shouldn’t be changed, remember we only rewrite JS requires / imports

dnolen12:07:56

@juhoteperi is warning namespace present in node_modules in your :output-dir?

dnolen12:07:11

@olivergeorge re: tests, yes please

juhoteperi12:07:55

@dnolen Yes, warning is available in node_modules, under react-leaflet but depending on it directly doesn't help

dnolen12:07:29

just trying to think through what’s going wrong

dnolen12:07:44

the error from Closure suggests that it doesn’t get added as a compiler input

juhoteperi12:07:48

If I understand index-node-modules-dir, it checks for main entry in package.json, but not browser

juhoteperi12:07:43

But if there is no main entry, it should use module-name.js or index.js

dnolen12:07:26

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.

juhoteperi12:07:24

Output from index-node-modules-dir looks correct, there is a entry which provides warning

dnolen12:07:31

@juhoteperi so maybe something about it that breakes Node.js resolution in Closure?

dnolen12:07:11

I do see that warning is CommonJS and the importing library is ES6 - maybe there’s problems with that in Closure?

juhoteperi12:07:15

The problem might be of our own creating, as we process ES6 and CommonJS separately

juhoteperi12:07:32

So when CommonJS is processed, Closure doesn't know about ES6 files etc.

dnolen12:07:35

@juhoteperi you can try the patch that changes that

dnolen12:07:31

if it works for you, please say so on the ticket - I’ll be looking at it later today

juhoteperi13:07:57

Doesn't fix this

juhoteperi13:07:22

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

juhoteperi13:07:53

And that both are :es6 when though the lib/ file should be commonjs?

juhoteperi13:07:23

only lib/index.js provides "react-leaflet", so I think the src files don't get processed

dnolen13:07:43

@juhoteperi with António’s patch the idea is that :module-type doesn’t actually matter anymore

dnolen13:07:33

including lib and src could be problematic but unclear yet

juhoteperi13:07:19

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

juhoteperi13:07:42

but that was changed by cljs-2279

juhoteperi13:07:34

(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"]}

juhoteperi13:07:10

So module_deps.js selects the file defined by browser entry in package.json

juhoteperi13:07:22

But perhaps Closure doesn't understand this

dnolen13:07:25

yes because of the target

dnolen13:07:33

ah right …

dnolen13:07:40

might not be a part of Node.js resolution

juhoteperi14:07:22

I wonder if module_deps.js should ignore browser entry for now, until Closure supports it?

dnolen14:07:41

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

juhoteperi14:07:23

I'll open a issue about this with notes

juhoteperi14:07:15

Replacing browserResolve with nodeResolve in module_deps.js seems to work

dnolen14:07:18

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

juhoteperi14:07:34

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

juhoteperi14:07:08

"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

thheller14:07:58

yes that is the error I mentioned above

thheller14:07:19

if you don’t process with AMD conversion it will transpile the code but that code won’t work

thheller14:07:47

compile error vs runtime error

juhoteperi14:07:04

Hmm, I think this is supposed to be UMD module, not AMD

thheller14:07:12

doesn’t matter

juhoteperi14:07:13

UMD wrapper that is

thheller14:07:20

some kind of wrapper that Closure doesn’t recognize 😉

juhoteperi14:07:32

Closure handles some of these, but this is a bit different

thheller14:07:34

there are many other fun things

juhoteperi14:07:38

enabling AMD support doesn't change this

thheller14:07:51

it does change it in that the compiler fails

thheller14:07:08

yes you won’t be able to use it but the compiler will tell you instead of pretending that everything is fine

juhoteperi14:07:29

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

juhoteperi14:07:41

But this error is nonsense because the define function should not be called for this module

juhoteperi14:07:09

It us UMD module so it can be directly used as CommonJS, IF the wrapper is correctly converted

thheller14:07:27

yes but closure does not recognize the wrapper

thheller14:07:37

since it does some custom stuff, so it won’t remove it

juhoteperi14:07:53

Did you check if there are existing Closure issues about not recognizing wrappers?

thheller14:07:30

leaflet in particular does some other things that are unlikely to work with closure

dnolen14:07:31

@juhoteperi but this might also be why browser is silly

dnolen14:07:39

UMD crap which we don’t care about

thheller14:07:10

it operates on a global L sometimes and things like that

juhoteperi14:07:22

Hmm, yeah. Not sure why Leaflet npm package uses UMD file as main entry

thheller14:07:40

I think its because of their custom plugin system

thheller14:07:32

which seemingly all assume that you load individual “.js” files via script tags

thheller14:07:02

its pretty similar to jquery in that regard

juhoteperi15:07:29

Woah, I modified the Leaflet wrapper to match Closure tests, and it works, without optimizations anyway

juhoteperi15:07:07

I think the different is that Leaflet wrapper uses ternary operator, and Closure only detects wrapper using if statements

dnolen15:07:54

@juhoteperi I believe Maria did the UMD detection stuff in Closure, I think her change could probably be made to work with ternary operator?

juhoteperi15:07:15

Yes, probably

juhoteperi15:07:43

I'll investigate a bit more before opening an issue at Closure-compiler

thheller15:07:16

bowser (required by material-ui) is another package with a weird wrapper

thheller15:07:35

!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 () {

juhoteperi15:07:17

Seem like module check is inverse of what Closure supports: typeof module === 'object'

juhoteperi15:07:04

A few new test cases and fixes will probably fix loads of packages

anmonteiro15:07:29

Hrm last time I checked our support for the browser field in package.json worked

juhoteperi15:07:00

Our support works, but Closure doesn't have support

dnolen15:07:54

@anmonteiro Closure node module resolution doesn’t take browser module into account

anmonteiro15:07:31

@dnolen got it, so should we disable it for now?

anmonteiro15:07:25

@juhoteperi btw index-node-modules-dir will always be right because it indexes every file in node_modules

anmonteiro15:07:48

So hopefully that will never be the source of problems

dnolen15:07:29

@anmonteiro I think providing a PR to Closure for at least the internal changes would be nice

dnolen15:07:43

someone else can sort out the command line details

anmonteiro15:07:58

I could try that

anmonteiro15:07:26

Got plenty of Chad's PRs to look at I guess

juhoteperi15:07:42

I commented at https://github.com/google/closure-compiler/issues/2433, with quick idea of where this would be implemented

dnolen15:07:09

@anmonteiro another thing that might make sense to do would be decouple ourselves from the Closure Compiler release cycle

dnolen15:07:26

not a full fork, but our own artifact

dnolen15:07:50

we already do this for Closure Library anyhow

anmonteiro15:07:53

So that we could apply our own patches

dnolen15:07:09

I’m less interested in that kind of drift

dnolen15:07:22

just when our patches land in Closure master

dnolen15:07:25

we don’t have sit around waiting for them

anmonteiro15:07:42

Yeah, that's what I meant sorry

anmonteiro15:07:53

Using master without being released

dnolen15:07:07

their release cycle is really only for the external world

anmonteiro15:07:11

Like the NPE patch they have in master

anmonteiro15:07:20

Which would be rly nice to have today

dnolen15:07:46

I’ll try to get this sorted out by this Friday

anmonteiro15:07:20

Btw re:cljs-2286 I still don't agree with setting processamd to true all the time

anmonteiro15:07:42

Because of the UMD problem which can be parsed as commonjs

anmonteiro15:07:09

So in my patch I'm only setting AMD processing to true if there's some AMD module in there

anmonteiro15:07:27

Which will be rare anyway

juhoteperi15:07:45

Sounds reasonable to me

juhoteperi15:07:59

The errors from AMD conversion don't make any sense with UMD files

anmonteiro15:07:21

And we have tests that show that too

anmonteiro15:07:29

In cljs.module-processing-tests

anmonteiro15:07:58

@juhoteperi the tests you first added last year actually

juhoteperi16:07:11

core-js has problems as it uses var meta = module.exports = { ... }, hmh, perhaps this was fixed by the recent change in Closure

juhoteperi16:07:24

Oops, by Closure build was broken and seems like this was fixed by the latest change

thheller17:07:21

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

juhoteperi17:07:57

Hmm, yeah, we already write the files to disk

juhoteperi17:07:57

Ah no, Closure is passed SourceFile which can be constructed from code (as string), File or InputStream

juhoteperi17:07:08

The files are written to disk only after Closure processing

juhoteperi17:07:35

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

thheller17:07:23

(SourceFile/fromCode name code)

juhoteperi17:07:01

... right 😄

thheller17:07:43

you construct the sourcefile instances before passing them to closure, so do the rewrite there. no need to touch files

dnolen18:07:59

hrm it looks like :reload :reload-all at the REPL is broken with 1.9.854?

dnolen18:07:34

meaning it doesn’t seem to force recompiles

Oliver George23:07:24

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.

Oliver George23:07:04

There are what look like a couple of bugs. I've noted them with FIXME comments in that patch for now.