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-dropzone enable importing as es6 module #17904

Closed
wants to merge 1 commit into from

Conversation

frankwallis
Copy link
Contributor

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:
If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: Importing as an ES6 module works using version 0.0.32 but was broken by this change: d1a4134
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@dt-bot
Copy link
Member

dt-bot commented Jul 10, 2017

types/react-dropzone/index.d.ts

to authors (@matdube @LynxEyes @goblindegook @benbayard). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@benbayard
Copy link
Contributor

Unfortunately react-dropzone project is not an ES6 module, it is compiled to commonjs. You should import this module using import Dropzone = require('react-dropzone');

@frankwallis
Copy link
Contributor Author

@benbayard importing the module using import Dropzone = require('react-dropzone'); means you cannot compile to ES6 modules so you cannot take advantage of webpack tree-shaking or use Rollup.

If you try to compile the require syntax to ES6 modules, typescript throws this error:
TS1202: Import assignment cannot be used when targeting ECMAScript 2015 modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.

Using import * as Dropzone from 'react-dropzone syntax is the only way you can make the code compile to both commonjs (for testing) and ES6 modules (for bundling) and work in both.

@benbayard
Copy link
Contributor

I'll wait for the DT team to comment here, but there is #17718 which is similar to this PR.

If this is blocking you I believe v0.0.32 used an ES6 compatible export.

@frankwallis
Copy link
Contributor Author

Fair enough but I would just point out that this makes it impossible to use commonjs modules and compile to to es2015 which is necessary to take advantage of webpack tree-shaking or use Rollup.

I will now have to pin these typings to 0.0.32 (which does support export *) and either get react-dropzone to export default or wait for microsoft/TypeScript#16093 to land.

@benbayard
Copy link
Contributor

@frankwallis does allowSyntheticDefaultImports work for you?

@frankwallis
Copy link
Contributor Author

@benbayard that enables using import Dropzone from 'react-dropzone' when bundling with webpack, but in development we output to commonjs so we can run the unit tests directly in nodejs and node does not support the synthetic default import so the code fails at runtime.

@RyanCavanaugh
Copy link
Member

I don't understand in what scenario you would want to use import * as X from 'Y' and then try to write X(). You're setting yourself up for future breakage.

@frankwallis
Copy link
Contributor Author

The scenario is:

  • you want to import a class (e.g. a react component) which is exported as a commonjs module
  • you also want to be able to cross compile to both es2015 and commonjs

I can see a couple of solutions to this:

  1. Deliver Synthesize namespace records on CommonJS imports if necessary microsoft/TypeScript#16093 which as I understand it will eliminate the need for import A = require(...) syntax, and solve the problem at runtime
  2. Relax the TS1202 restriction and allow import A = require(...) syntax when outputting es2015 modules. I not completely up to date with the current state of es2015 modules in NodeJS but I believe they are going to allow a mixture of imports and requires which may mean that this is needed in any case.

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.

4 participants