Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for map key local variable resolution. #162

Merged
merged 1 commit into from
Mar 26, 2017
Merged

Conversation

lynaghk
Copy link
Contributor

@lynaghk lynaghk commented Mar 25, 2017

See discussion in #160.

Note: I tried to verify that this test fails at 0.7.7 (638d3e7), but the tests failed to run:

$ lein doo node nodejs auto

Building ...
... done. Elapsed 15.323061661 seconds

;; ======================================================================
;; Testing with Node:

module.js:472
    throw err;
    ^

Error: Cannot find module 'react'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at /Users/kevin/software/sablono/target/nodejs/out/react-dom.inc.js:16:24
    at Object.<anonymous> (/Users/kevin/software/sablono/target/nodejs/out/react-dom.inc.js:40:3)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)

@r0man
Copy link
Owner

r0man commented Mar 25, 2017

@lynaghk Does lein deps help, before you run the tests? There should be a node_modules directory with the react dependencies.

@lynaghk
Copy link
Contributor Author

lynaghk commented Mar 25, 2017

@r0man Ah yes, that did it, and confirmed that the test fails at 0.7.7.

I didn't need to run lein deps when running the tests on this commit, though.
I was under the impression that there hasn't been a need to run lein deps manually since lein one-point-something.

If it's a sablono or plugin specific behavior, it may be worth adding that to the FAQ about running tests.

@r0man r0man merged commit 8377b21 into r0man:master Mar 26, 2017
@r0man
Copy link
Owner

r0man commented Mar 26, 2017

@lynaghk I think you are right, that lein deps is generally not necessary. If I remember correctly, it was necessary to get the npm dependencies installed via lein-npm.

However, I can't reproduce this anymore on my machine and Travis CI runs lein deps before building the project. I tried a fresh checkout and lein doo node nodejs auto is working as expected. I also don't have React installed in my global node_modules. Recently someone else had the same problem, so I added a note here:

I also noticed that lein-npm isn't needed anymore and the ClojureScript compiler seems to pick up React from cljsjs when running on Node.js. This wasn't the case in older versions of ClojureScript, or at least broken at some point. I removed the lein-npm dependency here:

Could it be that you had an older version of ClojureScript somehow on the classpath? I'm also using Leiningen 2.7.1.

Thanks for the test!

Roman

@lynaghk
Copy link
Contributor Author

lynaghk commented Mar 26, 2017

Could it be that you had an older version of ClojureScript somehow on the classpath? I'm also using Leiningen 2.7.1.

Nope, my lein profile only adds org.clojure/tools.namespace.
I'm on Leiningen 2.5.3, but don't see anything in the changelog that appears to be related.

A note in the FAQ should be fine, since relying on recent clojurescript compiler behavior would lead to trouble when trying to run tests to compare different sablono versions.
That may have been what happened in my case, since in 734ca01 cljs was updated to a newer version w/ javascript module support.

Anyway, thanks for the discussion and the quick fixes!
Do you have an ETA on the next release, or need any help on specific issues to get there?

@r0man
Copy link
Owner

r0man commented Mar 27, 2017

@lynaghk I just released 0.8.0.

@lynaghk
Copy link
Contributor Author

lynaghk commented Mar 27, 2017

@r0man Awesome, thanks!
Please let me know if you ever visit the American Pacific Northwest so I can buy you some drinks = )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants