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 default export #494

Merged
merged 3 commits into from
Sep 26, 2017
Merged

add default export #494

merged 3 commits into from
Sep 26, 2017

Conversation

huan
Copy link
Contributor

@huan huan commented Sep 23, 2017

Enable us to use import levelup from 'levelup' by adding default export.

@ralphtheninja
Copy link
Member

LGTM

@juliangruber
Copy link
Member

Should we document this as well? You too often have to guess how to import a node module

@ralphtheninja
Copy link
Member

@zixia Mind adding something to the README?

@huan
Copy link
Contributor Author

huan commented Sep 25, 2017

@ralphtheninja Ok, I will try to add some docs for this.

@huan
Copy link
Contributor Author

huan commented Sep 25, 2017

I just add some docs about to introduce the require/import and typing.

Do not know where is the best position to add them, so I put them after the Promise part.

@juliangruber
Copy link
Member

juliangruber commented Sep 25, 2017

I feel this could be simpler, what about having this in the first code example:

var levelup = require('levelup')
var leveldown = require('leveldown')

// or
import levelup from 'levelup'
import leveldown from 'leveldown'

@huan
Copy link
Contributor Author

huan commented Sep 25, 2017

@juliangruber No problem for me. I can modify the PR if needed.

Actually, this PR does not change any import/require usage than before.

It only affects the rollup tools behavior. (see my reference above, and this one: rollup/rollup#1267 (comment))

@juliangruber
Copy link
Member

Well, it adds the ability to import the default export for any es6 modules compliant environment.

README.md Outdated

We have two ways to import(require) the levelup module in the code.

### 1. The old fashioned `require`
Copy link
Member

Choose a reason for hiding this comment

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

By using require

README.md Outdated
var levelup = require('levelup')
```

### 2. The new ES6 `import`
Copy link
Member

Choose a reason for hiding this comment

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

By using ES6 import

README.md Outdated
import levelup from 'levelup'
```

## TypeScript
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove this section? Not sure what it has to do with this PR? (And we did remove typings for now until a later version)

@huan
Copy link
Contributor Author

huan commented Sep 25, 2017

Edited by following reviews from @ralphtheninja

@MeirionHughes
Copy link
Member

MeirionHughes commented Sep 25, 2017

I think typescript will have a hard time supporting both with this because you're mixing module kinds:

export = levelup;
export default levelup;

error:

[ts] An export assignment cannot be used in a module with other exported elements.

So to type this I'd have to pick one kind for ts users. Currently, ts users have:

import * as levelup from 'levelup';

switching over to the default export would mean ts-users would need to do:

import levelup from 'levelup';

This does work for me locally and moving forward, using the es6 standard appears to make sense.

I would caution though that the way es6 module loaders deal with es6 modules is not the same as commonjs: which is why nodejs still isn't switching over: https://medium.com/the-node-js-collection/an-update-on-es6-modules-in-node-js-42c958b890c

@huan
Copy link
Contributor Author

huan commented Sep 25, 2017

@MeirionHughes Yes, you can not use export = with export default together.

We should either to use

  1. in ES6/TypeScript
export { levelup }
export default levelup

or

  1. in traditional
export = levelup.default = levelup.levelup = levelup

I believe in both of above two kinds, the user could always use import * as levelup from 'levelup' in their code.

@juliangruber
Copy link
Member

It has to stay

const levelup = require('levelup')

for "older" systems though, is there a way to keep this and make es6 environments happy?

@ralphtheninja
Copy link
Member

I'm good with the changes and agree with @juliangruber, whatever you want to pick for es6 stuff should not mess up current functionality.

@huan
Copy link
Contributor Author

huan commented Sep 26, 2017

@juliangruber Yes, I agree with you that we have to stay const levelup = require('levelup').

This PR did not affect the "older" systems usage because it only adds a default export to the library.

@ralphtheninja ralphtheninja merged commit f93790f into Level:master Sep 26, 2017
@huan
Copy link
Contributor Author

huan commented Oct 2, 2017

When will we publish the latest version to NPM? Thanks!

@MeirionHughes
Copy link
Member

I've got the typings nearly ready to add back in to; waiting on leveldown.

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.

None yet

4 participants