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

Enhance import/default to validate require('...').default #754

Closed
gmathieu opened this issue Feb 28, 2017 · 6 comments
Closed

Enhance import/default to validate require('...').default #754

gmathieu opened this issue Feb 28, 2017 · 6 comments

Comments

@gmathieu
Copy link
Contributor

As we transition from commonJS to ES modules, we want to convert module.exports = ... to export default .... The challenge is detecting whether commonJS requires explicitly require('...').default.

I was thinking about creating a PR enhancing import/default with this functionality. If so, should it be on by default or configurable? If configurable, what should I name the option? Should this be a new rule altogether?

Note: I'd rather clean up the source then leverage something like babel-plugin-add-module-exports.

@ljharb
Copy link
Member

ljharb commented Feb 28, 2017

In our codebase, we generally approach this by 1) convert all requires to import and then 2) safely convert to export default.

Can you elaborate on what "this functionality" means? If you're referring to babel's .default, that's not spec, not universal, and not really something a linter can deal with :-/

@gmathieu
Copy link
Contributor Author

gmathieu commented Feb 28, 2017

@ljharb thanks for the tip.

For clarity, here's how I would update default.md:

```js
// ./foo.js
export default function () { return 42 }
```

The following is considered valid:

```js
var foo = require('./foo').default
```

...and the following cases are reported:

```js
var foo = require('./foo') // requiring ES module must reference default
```

Our legacy code uses a pattern I haven't been able to convert to ES modules:

function (model) {
  assert(model instanceof require('path/to/module'))
  ...
}

I don't believe there's a way to do inline import and moving those to the top might generate circular dependencies. Our best bet is to keep commonJS for inline requires and make sure require('/path/to/module').default is validated.

If the linter knows about export default here, could it be enhanced to check require statements?

@ljharb
Copy link
Member

ljharb commented Feb 28, 2017

You're correct that the only way to do inline import is import(), which is async (it returns a Promise). My code review hat would say that if hoisting them to the top would generate circular deps, they should be rearchitected to avoid that, but I'm aware that might be prohibitively difficult.

@gmathieu
Copy link
Contributor Author

@ljharb I opened a PR with a proposal. Let me know what you think. Thanks.

@gmathieu
Copy link
Contributor Author

gmathieu commented Mar 1, 2017

In our codebase, we generally approach this by 1) convert all requires to import and then 2) safely convert to export default.

@ljharb I thought I'd try your approach and hit a wall pretty quickly. Apparently Webpack 2 isn't so happy about mixing import and module.exports in the same file. The error is as follows: "Uncaught TypeError: Cannot assign to read only property 'exports' of object '#'". As soon as I replaced module.exports with export default, everything worked fine.

@ljharb
Copy link
Member

ljharb commented Jan 4, 2018

Closing per #755 (comment)

@ljharb ljharb closed this as completed Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants