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

Fix some linter issues, update ESLint #2318

Merged
merged 2 commits into from Nov 30, 2018

Conversation

2 participants
@isiahmeadows
Copy link
Collaborator

isiahmeadows commented Nov 29, 2018

Description

Motivation and Context

Having two exports is pretty redundant - you're only saving 2 non-whitespace characters by using a default export. Also, I fixed some linter issues I ran into. Edit: Noticed that our local ESLint version was outdated and that there were a few odd things missed. I also ran npm audit fix as a drive-by.

Saving the module changes for later once I have some bundler support.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@project-bot project-bot bot added this to Needs triage in Triage/bugs Nov 29, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

isiahmeadows commented Nov 29, 2018

Also, I ran npm audit fix on it to resolve a few problems.

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:bundler-render-fix branch from ab6cd20 to 5c6b980 Nov 29, 2018

@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Nov 29, 2018

I had the two exports there because the two question I've seen asked most about import of various libraries are:

why doesn't import X from 'x' work

why doesn't import { X } from 'x' work

Having a default export it's also simpler to write import mithril from 'mithril' than having to type import { m as mithril } from 'mithril'

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

isiahmeadows commented Nov 29, 2018

@porsager Ours is easy: we just export multiple things. If we export multiple things and we do so consistently, nobody will ask. We'll just need to document the ESM API being different than CommonJS due to idiomatic differences.

@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Nov 29, 2018

But doing both we don't have to document anything?

I'm really in favor of "it just works". Unless there are downsides I don't see?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

isiahmeadows commented Nov 29, 2018

@porsager I'm considering reverting the ESM change in favor of just updating the bundler to work with ES modules (I'm 99.99% it's possible to do surgically.)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator Author

isiahmeadows commented Nov 29, 2018

Edit: I'm going to go ahead and keep this strictly limited to linting.

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:bundler-render-fix branch from 5c6b980 to 4cbb543 Nov 29, 2018

@isiahmeadows isiahmeadows changed the title Fix some linter issues, remove the default `m` export. Fix some linter issues, update ESLint Nov 29, 2018

@isiahmeadows isiahmeadows merged commit 7c5024f into MithrilJS:next Nov 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Triage/bugs automation moved this from Needs triage to Closed Nov 30, 2018

@isiahmeadows isiahmeadows deleted the isiahmeadows:bundler-render-fix branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.