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

update react-modal #1319

Merged
merged 1 commit into from
Sep 7, 2017
Merged

update react-modal #1319

merged 1 commit into from
Sep 7, 2017

Conversation

hlolli
Copy link
Contributor

@hlolli hlolli commented Sep 6, 2017

Bump to version 2.3.1

Extern: Generated.
Update:

Api canged, now one needs to provide property for the RactModal calls (.-default js/ReactModal) for example. This is explained on the readme on their git repo ex. var Modal = (require 'react-modal').default

@Deraen
Copy link
Member

Deraen commented Sep 6, 2017

Where is use of ES6 default export documented? Doesn't make any sense, but I guess that is normal for JS libs.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 6, 2017

No, bullshit from my behalf, this is not in the README, sorry, blind faith.

I should add this info into the cljsjs.react-modal readme?

@Deraen
Copy link
Member

Deraen commented Sep 6, 2017

I'm quite sure their webpack build is broken. ES6 export default property is not supposed to be seen on the dist build.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 6, 2017

Ok, could be. This is where I actually found out about the .-default reactjs/react-modal#470 and this is 19 days old, the version Im PR-ing against is 17 hours old. So between at least 2 versions, they haven't fixed it. The problem with the current cljsjs version of react-modal is that it uses deprecated React.DOM.noscript and fails. I'll compile the the new version in advanced and confirm that it works or not. Writing .-default may be better of 2 bad options.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 6, 2017

Nah, looks like it's failing

TypeError: Cannot read property 'Component' of undefined
    at Object.defineProperty.value (eval at <anonymous> (:2:398), <anonymous>:97:19286)
    at t (eval at <anonymous> (:2:398), <anonymous>:97:444)
    at Object.t.exports (eval at <anonymous> (:2:398), <anonymous>:97:2088)
    at t (eval at <anonymous> (:2:398), <anonymous>:97:444)
    at Object.defineProperty.value (eval at <anonymous> (:2:398), <anonymous>:97:674)
    at t (eval at <anonymous> (:2:398), <anonymous>:97:444)
    at Object.defineProperty.value (eval at <anonymous> (:2:398), <anonymous>:97:532)
    at eval (eval at <anonymous> (:2:398), <anonymous>:97:537)
    at n (eval at <anonymous> (:2:398), <anonymous>:97:290)
    at Window.eval (eval at <anonymous> (:2:398), <anonymous>:97:312)

Hm, looks broken, twice they feed _react.Component to their function in their code, this could be the source (otherwise it's some other lib in my project causing this).

@Deraen
Copy link
Member

Deraen commented Sep 6, 2017

reactjs/react-modal#493

@hlolli
Copy link
Contributor Author

hlolli commented Sep 6, 2017

Ok, it's not failing, version 2.3.1 works. The reason I got this message is unknown to me, but I'm building a Greasemonkey user script, and there it failed, in a script tag on html page, it works.

@Deraen nice hehe, even better :) I'll watch this PR and react if they fix this and update my PR.

@hlolli
Copy link
Contributor Author

hlolli commented Sep 7, 2017

PR Updated to v2.3.2, regenerated the externes and squashed the two commits.

@Deraen Deraen merged commit 4d68647 into cljsjs:master Sep 7, 2017
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.

None yet

2 participants