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

Add prefer-default-export rule (fixes #304) #308

Closed

Conversation

gavriguy
Copy link
Contributor

@gavriguy gavriguy commented May 5, 2016

Add prefer-default-export rule

When there is only a single export from a module prefer using default export over named export.


// There is more thank one named exports in the module.
export const foo = 'foo';
export const bar 'bar';
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing = here

@jfmengels
Copy link
Collaborator

Thanks a lot @gavriguy, looks good!

Could you add a line about it in the README, and add an entry in the changelog? You can thank yourself there too :)

@gavriguy
Copy link
Contributor Author

gavriguy commented May 5, 2016

fixed. thanks for the warm welcome

@benmosher benmosher added this to the next milestone May 6, 2016
@benmosher
Copy link
Member

benmosher commented May 6, 2016

Just making a note: would like to support

  • count export { x, y, ... } as >=2 named exports
  • handle export { z as default } as a valid default export
  • add valid + invalid tests for these cases

I think the first is solved by counting ExportSpecifiers instead of ExportNamedDeclarations.

The latter just needs to inspect the exported.name on ExportSpecifiers and assert hasDefaultExport if specifier.exported.name === 'default'. or as code:

// replaces `ExportNamedDeclaration` visitor. 
'ExportSpecifier': function(node) {
  if (node.exported.name === 'default') {
    hasDefaultExport = true
  } else {
    namedExportCount++
    namedExportNode = node
  }
}

@benmosher
Copy link
Member

benmosher commented May 6, 2016

@gavriguy let me know if you're up for making those changes in this PR. I can also do it post-merge if you'd rather not. 😎

@gavriguy
Copy link
Contributor Author

gavriguy commented May 6, 2016

great ideas ill work on it. thanks

@gavriguy
Copy link
Contributor Author

gavriguy commented May 7, 2016

I've made the changes.
It was a bit more complicated than your suggestion as ExportSpecifier isn't called on all named exports - its only called when the named export is inside curly braces.
To solve this - I had to create two counters: namedExportCount & specifierExportCount.

Anyways now everything works and all tests are passing.

@gavriguy
Copy link
Contributor Author

gavriguy commented May 7, 2016

one of the build failed because of timeout - I think its a travis issue, but I don't see how to rebuild it again (maybe because I don't have the right permissions at Travis) Can you please have a look?

@benmosher
Copy link
Member

Changes look good, no worries on the build.

You could rebase against the tip, if you want, but I will probably tweak the change log a little before merging anyway, so you don't need to worry about it. Thanks for the updates! 😄

@gavriguy gavriguy force-pushed the add-prefer-default-export-rule branch from 93f4dd8 to 1e0992d Compare May 8, 2016 17:46
@gavriguy
Copy link
Contributor Author

gavriguy commented May 8, 2016

Thanks, I rebased against master - please let me know if you need anything more. thanks

@benmosher
Copy link
Member

Merged via cf45512. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants