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

@uppy/thumbnail-generator exifr.orientation is not a function #2172

Closed
EtienneBruines opened this issue Apr 3, 2020 · 7 comments · Fixed by #2195
Closed

@uppy/thumbnail-generator exifr.orientation is not a function #2172

EtienneBruines opened this issue Apr 3, 2020 · 7 comments · Fixed by #2195
Labels

Comments

@EtienneBruines
Copy link

EtienneBruines commented Apr 3, 2020

The error

Upon requesting a thumbnail image, the following error is thrown:

TypeError: exifr.orientation is not a function
    at ThumbnailGenerator.createThumbnail ((index):44693)
    at ThumbnailGenerator.requestThumbnail ((index):44917)

This points to this line:

    const orientationPromise = exifr.orientation(file.data).catch(_err => 1)

Related commits

fc3b50a
a6bfd81

@MikeKovarik ?

My code

thumbnailGenerator.requestThumbnail(uppyFile);

Versions

Compiled with typescript.

Broken:

@uppy/thumbnail-generator: 1.5.7
@uppy/dashboard: 1.8.0
exifr: 4.3.5

Broken:

@uppy/thumbnail-generator: 1.5.6
@uppy/dashboard: 1.7.0
exifr: 4.3.4

Working:

@uppy/thumbnail-generator: 1.5.5 (manually locked; 1.5.7 will be auto-installed)
@uppy/dashboard: 1.6.2
exifr: not installed
@EtienneBruines
Copy link
Author

EtienneBruines commented Apr 3, 2020

For some weird reason, exifr is seen as an empty-object {} as by console.log and debugger.

For some reason, these seem to work:

const exifr = require('exifr')

Within the lib/index.js file, changing the import from .cjs to .js also seemed to do the trick 😕

@EtienneBruines
Copy link
Author

Okay, rollup with default commonjs could not handle cjs...

    // rollup.config.js
    commonjs({
      extensions: ['.js', '.cjs']
    }),

@MikeKovarik
Copy link
Contributor

MikeKovarik commented Apr 3, 2020

Hello, i was about to look into it :D sorry for the inconvinience.

Exifr is written as ESM and transpiled to both ESM and UMD. But since the current disarray with js/mjs/cjs and "type":"module" in package.json i decided to include each bundle (mini, lite, full) in two identical variants with both js/mjs and js/cjs extensions. So there's dist/mini.umd.js === dist/mini.umd.cjs, dist/lite.esm.js === dist/lite.esm.mjs, etc...

In the PR I decided to go with cjs, but i didn't expect it cause any problem since it would compile and work just fine. Anyway, I'm glad it's fixed 👍

@apuyou
Copy link
Contributor

apuyou commented Apr 13, 2020

Hi !

I have the same error message:

TypeError: exifr.orientation is not a function

I am using https://github.com/facebook/create-react-app, so I can't change any config, but it does seem like the issue is importing a .cjs file directly.

I have tried replacing the require statement with exifr instead of `exifr/dist/mini.legacy.umd.cjs. The resolution happens correctly, but then we are not getting the mini edition of exifr.

Do you think that CRA should support .cjs imports or is there anything we can do to fix it from this side?

Please tell me if you think I should put this in a new issue.

Thanks!

@goto-bus-stop
Copy link
Contributor

CRA should support .cjs imports, but since it's still causing trouble perhaps we should use the .js import in Uppy for now, until most tools are properly updated.

@apuyou
Copy link
Contributor

apuyou commented Apr 13, 2020

I have just created a new CRA project from scratch to isolate the issue and confirmed that there is nothing linked to my project: https://github.com/apuyou/uppy-thumbnail-test

Thank you for the suggestion on just changing the extension @goto-bus-stop. I'll open a PR.

@MikeKovarik
Copy link
Contributor

MikeKovarik commented Apr 14, 2020

I just commented on this in the #2195 but I'll copy-paste it here for SEO reasons :D


Hello, yes I have been looking into it earlier today but hadn't had time to reply yet.

I know why the ERR_REQUIRE_ESM error happens. I wrote the library as a forward looking ES module with the new official "type":"module" way to define ES packages. Unfortunately this isn't yet widely supported so I added .cjs and .mjs copies of bundles. .cjs is there mainly for node.js (as Node 13 understands it and older Node just interprets it as CJS anyway) whereas .js is there mainly for web servers that don't understand .cjs (nor .mjs) since it's an invention of Node.

Funny thing happened here. .cjs is not yet understood by tools, but then the .js variant is falsely recognized as ESM due to "type":"module" in package.json.

The solution: I will release new version without "type":"module" (since source files are already renamed to .mjs and "main" now points to UMD aka CJS for wider support) and it will just work :D. I was already planning on doing this but didn't want to potentially break anything. But releasing major version will do the trick.

What a horrible time to be writing ESM :D

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

Successfully merging a pull request may close this issue.

4 participants