Fork me on GitHub
#cljs-dev
<
2017-08-14
>
symfrog10:08:55

@anmonteiro I created a repo that reproduces the issue: https://github.com/symfrog/cljs-modules-react-order . I think the cause is related to the necessary dependency exclusion here https://github.com/symfrog/cljs-modules-react-order/blob/master/project.clj#L5 . Running lein deps :tree shows the following dependencies: [cljsjs/react-dom "15.5.4-0" :exclusions [[cljsjs/react]]] [cljsjs/react-with-addons "15.5.4-0"] cljsjs/react-dom requires cljsjs.react in its deps.cljs and cljsjs/react-with-addons provides cljsjs.react in its deps.cljs.

mhuebert11:08:46

I ran into more emoji-related bugs with self-hosted compiler, and subsequently found bugs in my previous fix for https://dev.clojure.org/jira/browse/CLJS-1576. Just uploaded a new patch.

juhoteperi12:08:31

@symfrog I recommend not using react-with-addons, it is deprecated

juhoteperi12:08:12

> React-with-addons bundle [has been deprecated](https://facebook.github.io/react/docs/addons.html) and Cljsjs no longer provides new versions > of that package. The latest React-with-addons version won't work with Reagent 0.8. > For animation utils use [react-transition-group](https://github.com/cljsjs/packages/tree/master/react-transition-group) package instead (TODO: Update to use :global-exports). [React-dom/test-utils] > (https://facebook.github.io/react/docs/test-utils.html) and [react-addons-perf](https://facebook.github.io/react/docs/perf.html) are not currently packaged as browserified files, so their use would require Webpack, or they might work with Closure module processing (TODO: Provide example). https://github.com/reagent-project/reagent/blob/master/CHANGELOG.md

symfrog12:08:31

@juhoteperi Thanks, I have already switched and I am not using react-with-addons, just using it to illustrate the potential bug

dnolen12:08:01

@symfrog dependency exclusions can’t possibly matter

dnolen12:08:14

unless your deps have bad deps.cljs, then not our problem

dnolen12:08:57

the issue needs to be reproducible with something more minimal than this

symfrog12:08:21

@dnolen The deps.cljs seem to be correct in both cljsjs/react-with-addons and cljsjs/react-dom, both are cljsjs packages and are the only dependencies specified other than clj/cljs. I will try to produce something more minimal.

anmonteiro15:08:42

I did try that repo, david's fork of it and my own attempt of reproing the bug

anmonteiro15:08:01

Couldnt repro with either master or 1.9.854 in all combinations

mfikes15:08:13

@anmonteiro FWIW, CLJS-2318 caused a failure for my CI (which I can’t repro locally): https://travis-ci.org/mfikes/clojurescript/builds/264352764#L1941-L1943

anmonteiro15:08:59

But the next commit passed

anmonteiro15:08:12

Oh this is not CLJS-2309

anmonteiro15:08:01

Looks like a corrupted download

anmonteiro15:08:07

Checksum mismatch etc

mfikes15:08:44

OK. That could likely explain it. I’ll have it do another build without cache to ensure it doesn’t repro again.

mfikes18:08:30

^ subsequent re-build succeeded, so this was likely indeed just a transient error owing to a botched download checksum

anmonteiro18:08:41

no reason for it to fail. It had passed multiple times before in both your CI and mine

anmonteiro18:08:01

glad we’re not going insane 🙂

anmonteiro20:08:30

@dnolen can we use :main with :modules?

anmonteiro20:08:09

so what I’m seeing is not a bug then 🙂

symfrog20:08:04

@anmonteiro A clean clone of https://github.com/symfrog/cljs-modules-react-order followed by running ./scripts/release outputs React DOM before React in ./release/mobuleb.js on my side. I have not been able to reproduce this with more minimal cases, so it might be something specific to that repo or I might just not be spotting the cause to extract into a more minimal case.

anmonteiro20:08:05

@symfrog just doesn’t happen for me

anmonteiro20:08:12

which OS are you on?

symfrog20:08:49

OS X 10.11.6

anmonteiro20:08:40

I added a test case that should reproduce your problem

anmonteiro20:08:57

but CI passes

anmonteiro20:08:30

as I said above, I also did a checkout of your repo and David’s fork

anmonteiro20:08:32

and just couldn’t repro

symfrog20:08:07

@anmonteiro Thanks, I saw that and extracted that test into packaged library equivalents with deps.cljs, and could still not reproduce the issue

anmonteiro20:08:33

I’m happy to look at a more reliable repro if you have one

symfrog20:08:34

I also suspected that cljs.closure/get-upstream-deps might be returning incorrect values for that specific case, but it returned the correct values

anmonteiro20:08:00

your use case may be too conflated with tooling which we don’t care about

anmonteiro20:08:12

it just introduces too many variables

symfrog20:08:24

yes, could be the case, I produced various other minimal cases without being able to reproduce