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: export cjs and es #1298

Merged
merged 5 commits into from May 1, 2021
Merged

fix: export cjs and es #1298

merged 5 commits into from May 1, 2021

Conversation

simllll
Copy link
Contributor

@simllll simllll commented Apr 17, 2021

as commented in #1266, this one would restore the old behaviour and introduces a new entrypoint for es modules.

for CJS (javascript):
const Agenda = require('agenda');

or for module (typescript):
import Agenda from 'agenda/es'

This is a "another" breaking change though, as a named import would not work with the default entrypoint anymore (didn't work before, but did now in the patch releases)

@simllll simllll requested a review from leonardlin April 17, 2021 09:31
@simllll simllll requested a review from koresar April 17, 2021 09:46
leonardlin
leonardlin previously approved these changes Apr 18, 2021
Copy link
Contributor

@leonardlin leonardlin left a comment

Choose a reason for hiding this comment

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

I'm not an expert on module/exports etc. therefore I yield to other people.
But I added note for people migrating from @types/agenda

For Typescript, Webpack or other module imports, use `agenda/es` entrypoint:
e.g.
```ts
import { Agenda } from 'agenda/es';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make a note that people migrating from @types/agenda should change their imports
from: import Agenda from 'agenda'
to: import { Agenda} from 'agenda/es'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added it to the readme

leonardlin
leonardlin previously approved these changes Apr 20, 2021
@thebestnom
Copy link
Contributor

thebestnom commented Apr 22, 2021

Couldn't you just add

export = Agenda

In index.ts
Edit:
Nevermind, you can't do default and export = in the same file

@simllll
Copy link
Contributor Author

simllll commented Apr 22, 2021

Couldn't you just add

export = Agenda

In index.ts?

Unfortunately not, this would overwrite all other exports and would therefore not work correctly. It would only fix the plain JS includes, but this does my PR too.

@simllll
Copy link
Contributor Author

simllll commented May 1, 2021

Thanks for the additional fixes and tests! Lgtm too, feel free to merge

@koresar koresar merged commit 849b32f into master May 1, 2021
@koresar koresar deleted the fix/es6-exports branch May 1, 2021 23:56
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