Skip to content

Conversation

remcohaszing
Copy link

@remcohaszing remcohaszing commented Sep 19, 2024

What kind of change does this PR introduce?

  • Bug Fix
  • Feature
  • Refactoring
  • Style
  • Build
  • Chore
  • Documentation
  • CI

Did you add tests for your changes?

  • Yes, my code is well tested
  • Not relevant

If relevant, did you update the documentation?

  • Yes, I've updated the documentation
  • Not relevant

Summary
The main entrypoint of this package exports a function using module.exports = …. The correct type for this is export = ….

Does this PR introduce a breaking change?
No

Other information

Fixes #29

The main entrypoint of this package exports a function using
`module.exports = …`. The correct type for this is `export = …`.

Fixes #29
@okonet
Copy link
Collaborator

okonet commented Sep 19, 2024

This only changes the types. Is this really solves the problem with CJS? Could we have a test added?

@remcohaszing
Copy link
Author

Sorry, my bad. I misunderstood the issue. This package does use module.exports.default instead of module.exports. Although I recommend against doing that, the types are correct.

@okonet
Copy link
Collaborator

okonet commented Sep 19, 2024

I'm happy to accept a PR that solves the root cause.

@remcohaszing
Copy link
Author

How to solve the root cause depends on how you want the library to work. It’s going to be breaking anyway, so a semver major would be needed.

Do you want to keep dual publishing? If not, do you want CJS or ESM? If yes, the simplest solution is to switch to a named exports.

@remcohaszing remcohaszing deleted the fix-types branch September 19, 2024 13:21
@okonet
Copy link
Collaborator

okonet commented Sep 19, 2024

Would names exports solve the problem? I'd be happy to go with this one.

How much value dual publishing brings in 2024?

@remcohaszing
Copy link
Author

How much value dual publishing brings in 2024?

It really depends on who you ask.

I think the whole dual module system situation is a weird situation that we as the JavaScript community should try to get rid of. Personally I believe the only way to do this, is by pushing forward for ESM-only. There are also some projects that can only consume ESM, but CJS, but that’s relatively rare.

There are still a lot of projcts using CJS. For them it’s really convenient to be able to use require('attr-accept') instead of using an asynchronous dynamic import. Node.js 22 has experimental support of require()ing ESM as well, so in the future that will likely be possible.

You could dual publish to please everyone. Unfortunately that means you have a build step and you need to be careful about compatibility. When dual publishing you should be careful about the dual package hazard, though that’s not a problem for attr-accept.

The best overview of the state of CJS vs ESM can be seen here. So among popular packages, CJS is still used more, but ESM is gaining traction. This includes old packages that may be unmaintained and are stuck on CJS forever.

ESM vs CJS usage


Would names exports solve the problem? I'd be happy to go with this one.

CJS only allows exporting one member, whereas ESM allows exporting multiple members. Default exports make it extra weird, because how do you make an ESM default export compatible with CJS, if the ESM version could potentially export additional named members?

Named exports solve this, because it’s obvious the CJS export is an object that can be destructured nicely.

Also I think nowadays named exports are just generally more popular than default exports, but I don’t have numbers for that.

@okonet
Copy link
Collaborator

okonet commented Sep 23, 2024

I feel the same way so I'd be happy to get a PR that would be ESM only.

@remcohaszing remcohaszing mentioned this pull request Oct 4, 2024
12 tasks
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.

use : var accept = require('attr-accept'); accept() accept is not a function

2 participants