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

4.0.2 Dist Build Is Broken as result of latest ES6 build changes #190

Closed
virgofx opened this issue Aug 5, 2018 · 15 comments
Closed

4.0.2 Dist Build Is Broken as result of latest ES6 build changes #190

virgofx opened this issue Aug 5, 2018 · 15 comments

Comments

@virgofx
Copy link
Contributor

virgofx commented Aug 5, 2018

The changes for the ES6 build no longer work when including ReactImageCrop statically

<Removing bad copy/paste ... see below>

Last working build 4.0.1

@sekoyo
Copy link
Owner

sekoyo commented Aug 5, 2018

Hi you're looking at the wrong library ;)

https://unpkg.com/react-image-crop@4.0.2/dist/ReactCrop.js

@virgofx
Copy link
Contributor Author

virgofx commented Aug 5, 2018

Sorry wrong copy paste ... More clarification below, hopefully this helps?

4.0.1 (Working:

window.ReactCrop

ƒ ReactCrop() {
    var _ref;

    var _temp, _this, _ret;

    _classCallCheck(this, ReactCrop);

    for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key++) {
      args[…

4.0.2 (Not Working):

window.ReactCrop
{ReactCrop: ƒ, default: ƒ, getPixelCrop: ƒ, makeAspectCrop: ƒ, __esModule: true}
render() {
    const { ReactCrop } = window;

   <ReactCrop src={ .... } />
}

Error
React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: object.

@sekoyo
Copy link
Owner

sekoyo commented Aug 6, 2018

Interesting, I wonder how you would access other exports with the < 4.0.1 version e.g. getPixelCrop! How it is in 4.0.2 would be how I would expect it to be for a global one as I don't know how you would access the other exports otherwise

@virgofx
Copy link
Contributor Author

virgofx commented Aug 6, 2018

For dist builds it needs to be accessed via property ... example below. The main export needs to be a component/function. So we need to revert the change and republish or fix (Not sure about the new ES changes?)

render() {
    const { ReactCrop } = window;

   <ReactCrop 
        src={ .... }
        onImageLoaded={image => {
          const newCrop = ReactCrop.makeAspectCrop(
              {
                  x: 0,
                  y: 0,
                  aspect: 1,
                  width: 100,
                  height: 100,
              },
              image.width / image.height
          );
          this.pixelCrop = ReactCrop.getPixelCrop(image, newCrop);
      }} 
    />
}

Also noticed (not sure if intentional) when comparing the diff ... was containCrop removed ?
afddbaf

@sekoyo
Copy link
Owner

sekoyo commented Aug 6, 2018

Right gotcha

@sekoyo
Copy link
Owner

sekoyo commented Aug 6, 2018

@virgofx
Copy link
Contributor Author

virgofx commented Aug 6, 2018

Works! TY

@sekoyo sekoyo closed this as completed Aug 6, 2018
@sekoyo
Copy link
Owner

sekoyo commented Sep 25, 2018

@virgofx I've looked into this e.g. https://stackoverflow.com/questions/52450319/how-to-export-a-module-which-works-globally-and-for-es6-importing

And think I have no choice but to export as an object to solve a number of issues. So in the next major version it will have to be globally used as ReactCrop.default unfortunately

@virgofx
Copy link
Contributor Author

virgofx commented Sep 25, 2018

I'm not sure what you're trying to solve .. but this isn't how the industry publishes libraries. There is best practice for proper exports such that libraries can be used universally using Webpack and rollup... if it helps you could literally clone a lot of sample libraries that offer the library flavor for plugins for react. It needs to work globally so it can be included via UMD / browser / CDN. You're basically saying I want to do something different than the other 1,000,000 libraries published out there. Why? I mean look at actual React dist .. or any other library.

I'm not a module/export .. so I'm not sure what you're trying to achieve; however, I do know if you need a module specific export ... you just create a new export so for example /dist would look like this:

https://unpkg.com/reactstrap@6.4.0/dist/

In the above example there are 3 types: UMD files (the actual UMD is omitted since UMD should always be the main version) and two other types (CJS) and (ES) ... where ES is the pure module version (I believe). There may be examples of other starter libraries with rollup/webpack (not sure what's being used here) where you could modify the build process to be more standardized? Might be worth a shot I'll try to find some of those.

Again ... the primary version should always be UMD ... such that doing ReactCrop.blah should work from browser when included.

If that's the case you just need to modularize the build process to include multiple outputs.

@sekoyo
Copy link
Owner

sekoyo commented Sep 25, 2018

I have looked at them and they are all exported as objects so actually this is going against standards:

http://jsfiddle.net/ferahl/q42wtdrg/2/

@virgofx
Copy link
Contributor Author

virgofx commented Sep 25, 2018

I'm not sure what the standards are and I don't follow the fiddle; however, if you browse libraries .. they all export multiple types --- including a UMD .. so we need UMD so leave it .. Just add what you need without impacting anything else.

@virgofx
Copy link
Contributor Author

virgofx commented Sep 25, 2018

The sources files don't have to change for the ES exports. Again, it can be reconfigured via rollup/ webpack.

@sekoyo
Copy link
Owner

sekoyo commented Sep 25, 2018

The modules are still exported as UMD the export style isn't under question, as my SO question demonstrates nobody has the answer except perhaps exporting many build types and hoping that compilers read the correct one, which I don't really have the inclination to do since almost nobody uses global scripts and if they do it will work but they have to add default to it, something you would have to do with e.g. redux-saga

@sekoyo
Copy link
Owner

sekoyo commented Sep 25, 2018

Just tried most of the most popular react libs and only one of them exports as a function (ReactGridLayout) which is also the only one that uses module.exports from what I saw - http://jsfiddle.net/ferahl/q42wtdrg/17/

@virgofx
Copy link
Contributor Author

virgofx commented Sep 25, 2018

I wish I understood better (I'm not an export on export/import and what you're trying to achieve) ... All I know is that there should be a UMD version that is accessible to be imported via CDN (e.g. https://reactjs.org/docs/cdn-links.html) This allows application developers to exclude the specific librar(ies) ... and typically larger libraries from their application bundle and include it statically.

So currently, looks like ReactCrop currently only has the UMD build in dist (https://unpkg.com/react-image-crop@5.0.0/dist/). (Which is fine ... this should always be the primary)

I think it's pretty straight forward to add additional Rollup/Webpack output type (for ES/CJS) ... and then you specify the different versions that get picked up when using this library in your own application by updating the corresponding package.json fields which are used by different services (Rollup/Webpack/etc) in this library appropriately.

Here is an example:
https://github.com/reactstrap/reactstrap/blob/master/package.json#L5-L11

So again ... the issue isn't with removing UMD ... it's understanding what you're trying to do and what's not working in how your using this library in your separate application ... as I believe that's probably where the configuration error is.

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

No branches or pull requests

2 participants