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

[react-datepicker] Update to react-datepicker-0.53.0 #1304

Merged
merged 1 commit into from
Aug 28, 2017

Conversation

tomjkidd
Copy link
Contributor

@tomjkidd tomjkidd commented Aug 24, 2017

Update:
Extern: I updated the extern with generator.

  • Update README to include new version and show a factory creation example
  • Remove tether and scss_lint because they are no longer dependencies
  • Update checksum to match 0.53.0 zipfile
  • Update extern file

Worth mentioning that between the existing version (0.41.1) and this, I use the same boot build command, but have to use DatePicker.default in client code instead of just DatePicker.

* Update README
* Remove tether and scss_lint because they are no longer dependencies
* Update checksum to match 0.53.0 zipfile
* Update extern file
@Deraen
Copy link
Member

Deraen commented Aug 24, 2017

Worth mentioning that between the existing version (0.41.1) and this, I use the same boot build command, but have to use DatePicker.default in client code instead of just DatePicker.

That sounds wrong. Could be something on the webpack config...

I think good option would be to rewrite the script to download files from https://unpkg.com/react-datepicker@0.53.0/dist/ (official NPM cdn) as using existing files is less error prone and simpler. create-react-class is one package that provides example of that: https://github.com/cljsjs/packages/blob/master/create-react-class/build.boot

@tomjkidd
Copy link
Contributor Author

tomjkidd commented Aug 24, 2017

@Deraen I followed your advice, trying out externs generation for the unpkg.com version you linked to. Same results, so I started with 0.41.0 and stepped through releases to figure out where things changed.

Hacker0x01/react-datepicker@v0.44.0...v0.46.0#diff-f662ae56197d5c888897a02b982f0ad3L17

So, I can link to the unpkg.com 0.53.0 and simplify the build.boot file as you suggested, but as far as I can tell the new extern and using DatePicker.default are necessary. Any advice?

@Deraen
Copy link
Member

Deraen commented Aug 28, 2017

Have you tried loading the new file on browser? I tested it and it can't be loaded because it depends on react-onclickoutside: https://github.com/Hacker0x01/react-datepicker/blob/master/webpack.config.js#L48, but that package doesn't provide browserified JS. I have no idea how this lib is supposed to be used without webpack now.

https://codepen.io/anon/pen/eEPgPp

The default name on externs doesn't mean anything. It is still possible the JS code should be written without it, it is possible the extern generator is just confused about the code.

@Deraen
Copy link
Member

Deraen commented Aug 28, 2017

Asked about this upstream: Hacker0x01/react-datepicker#1003

@tomjkidd
Copy link
Contributor Author

react-onclickoutside is listed as an external dependency by react-datepicker, you have to provide it.
I forked from your codepen in order to create a minimal demonstration of a working version.
Note that the dependencies are the ones that I specified in the extern generator, and I print out DatePicker initially, to show that it has es6 module wrapping, then I reassign it to the function that default points to.

https://codepen.io/tomjkidd/pen/ayRWqx

@Deraen Deraen merged commit 3c1a8bd into cljsjs:master Aug 28, 2017
@Deraen
Copy link
Member

Deraen commented Aug 28, 2017

Aha, yes, up-to 6.0 react-onclickoutside provides browser compatible file.

Well, seems the code indeed requires use of default to access the function. JS libs are strange.

@tomjkidd tomjkidd deleted the react-datepicker-0.53.0 branch August 28, 2017 19:18
@Deraen
Copy link
Member

Deraen commented Sep 6, 2017

Found the reason for default export, same fix for React-modal: reactjs/react-modal#493

@tomjkidd
Copy link
Contributor Author

tomjkidd commented Sep 7, 2017

Oh, cool. Thanks for following up on this!

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