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 support for proper ES module #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

niksy
Copy link
Contributor

@niksy niksy commented May 4, 2021

This PR adds support for proper ES module configuration:

  • exports field which references CommonJS and ES module
  • Adjusts Rollup configuration to create additional package.json files with proper type properties

This should probably be breaking change as explained in official documentation.

@gpoitch
Copy link
Owner

gpoitch commented May 4, 2021

Hey @niksy could you explain what this does or solves for you? My understanding is exports is for submodules/multiple entries but this only has 1 so the main/module should cover it.

@niksy
Copy link
Contributor Author

niksy commented May 5, 2021

You’ve added module property to package.json but that doesn’t automatically makes it proper ES module. Minimal combination of type and exports field will make module ESM compatible.


Reproduction case:

package.json

{
  "type": "module",
  "scripts": {
    "start": "node --input-type=module -e 'import \"./index.js\";'"
  },
  "dependencies": {
    "viewprt": "^4.2.0"
  }
}

index.js

import { ElementObserver, ObserverCollection } from 'viewprt';

Running npm start will produce following error:

file:///Users/niksy/Projects/viewprt-test/index.js:1
import { ElementObserver, ObserverCollection } from 'viewprt';
         ^^^^^^^^^^^^^^^
SyntaxError: The requested module 'viewprt' does not provide an export named 'ElementObserver'

Applying changes from this PR will properly use module as ES module.

This is probably nitpicking since Rollup and Webpack will use module property, but making this proper ES module will align it more closely with specification.

Current proper usage in ESM mode, without any changes, is as follows:

import * as viewprt from 'viewprt';

const { ElementObserver, ObserverCollection } = viewprt

This works, but doesn’t tree-shake PositionObserver which is also exported.

@gpoitch
Copy link
Owner

gpoitch commented May 5, 2021

Thanks for explaining @niksy. So it's native node support. However this is a browser only package and there is no bundling done by node where it would have tree shaking. I guess it doesn't hurt to add incase some bundlers start using it. Can it be done without creating the duplicate package.jsons? (and thanks for researching, I'm just trying to understand it).

@niksy
Copy link
Contributor Author

niksy commented May 6, 2021

@gpoitch yeah, as I said, this doesn’t do anything for bundlers since they will take anything and resolve it to best possible case, but they already support it (see Rollup and Webpack related issues) so it doesn’t hurt to have package aligned with official specification.

As for duplicate package.json: yes, the other solution is to change ES module extension to .mjs. This automatically switches to compliant specification (see official documentation).

I will make changes to PR.

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.

2 participants