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

Discourage the use of default exports #274

Merged
merged 2 commits into from
Feb 18, 2020
Merged

Discourage the use of default exports #274

merged 2 commits into from
Feb 18, 2020

Conversation

perrupa
Copy link
Contributor

@perrupa perrupa commented Jan 20, 2020

IMO default exports confuse things by creating 2 ways to import/export components while offering no perceivable benefit/advantage.

I believe a world with only named imports would be more consistent, but I’m happy to hear and and all thoughts and opinions that are different (that's what PRs are for!).

polaris-react already enforces they not be used with the import/no-default-export rule (Shopify/polaris#1959) and web-admin also shies away from them.

Let's document it and make it official, OR be more specific about when and how they're used so we're consistent across repos.

Copy link

@jgodson jgodson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this 👍. Might make sense to introduce the lint rule at some point too in web. I think the only case where we might need default exports is when using react-async

README.md Outdated

```js
// bad
const BadImport = require('./BadImport');
module.exports = BadImport.feelBadAboutIt();
export default BadImport();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is having cjs and esm style exports in a single example confusing as this is invalid syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parens were already there, I just pushed up a commit removing them.

Does it read any better to you now?

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm onboard with telling people to use named exports, but input from a few more people across the org before we say "yep, everyone should do this" would be nice.

@lemonmade
Copy link
Member

While I have personally grown to prefer named exports in most cases, they are occasionally still quite handy. In particular, many tools that operate on async imports, like React's lazy() and our own createAsyncComponent, have special handling of default exports. It isn't really practical to do the same for named exports, because there's no reasonable default export (hence why the language construct exists).

Personally, my vote would be to leave it up to developers to do the thing that works for them, but if it's going to be something people won't stop debating in PRs, I guess a rule would be better.

@jgodson
Copy link

jgodson commented Jan 20, 2020

Personally, my vote would be to leave it up to developers to do the thing that works for them, but if it's going to be something people won't stop debating in PRs, I guess a rule would be better.

I think it's reasonable to have a recommended way of doing it (this PR). Maybe we don't need to enforce it with a rule though. Perhaps using wording like Prefer instead of Always?

It may warrant a new rule over modification of this rule perhaps if we go that route?

README.md Outdated Show resolved Hide resolved
@lemonmade
Copy link
Member

If you want to go that route, you'll need more nuance in the recommendation, and if you opt not to lint, we won't be able to enforce it in tooling so you probably won't get the discussion to go away (if that's what you're after).

@perrupa
Copy link
Contributor Author

perrupa commented Jan 22, 2020

If you want to go that route, you'll need more nuance in the recommendation, and if you opt not to lint, we won't be able to enforce it in tooling so you probably won't get the discussion to go away (if that's what you're after).

@lemonmade, yeah linting will always be up to the repo owners but I was hoping to have some docs to point to in these cases as best practice. I'm totally open to adding exceptions (like you mentioned) if you think that's warranted 👍

input from a few more people across the org before we say "yep, everyone should do this" would be nice

@BPScott I'm good with that, I'll put up a poll in #fed to gauge opinions.

Perhaps using wording like Prefer instead of Always?

@jgodson reasonable, I think this makes a lot of sense if there are good exceptional cases (like Chris mentioned).

@perrupa perrupa merged commit 2d5dfc2 into master Feb 18, 2020
@perrupa perrupa deleted the no-default-exports branch February 18, 2020 22:11
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.

5 participants