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

Remove default exports & update tests #513

Merged
merged 1 commit into from
Jun 8, 2022
Merged

Conversation

davidgoli
Copy link
Collaborator

Fixes #478

Supercedes #511

@codecov-commenter
Copy link

Codecov Report

Merging #513 (5d649be) into master (0733a7b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
- Coverage   88.49%   88.48%   -0.01%     
==========================================
  Files          29       29              
  Lines        2094     2093       -1     
  Branches      584      584              
==========================================
- Hits         1853     1852       -1     
  Misses        241      241              
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0733a7b...5d649be. Read the comment docs.

@davidgoli davidgoli merged commit 8f8630b into master Jun 8, 2022
@davidgoli davidgoli deleted the remove-default-exports branch June 8, 2022 12:48
@karlhorky karlhorky mentioned this pull request Jun 8, 2022
3 tasks
@karlhorky
Copy link

karlhorky commented Jun 8, 2022

Thanks for this @davidgoli ! I imagine will be soon published a part of a future 2.7.1 release?

As soon as it's out I'll test whether I can remove the patch I mentioned in #478

@karlhorky
Copy link

@davidgoli Will this be released sometime soon? Would love to try it out and remove my patch.

@karlhorky karlhorky mentioned this pull request Jun 30, 2022
7 tasks
@karlhorky
Copy link

@davidgoli just wanted to check in again - do you think this will be released soon?

@karlhorky
Copy link

Looks like it was published in version 2.7.1, and this version indeed resolves the types issue with TypeScript and ESM.

However, since rrule is still a CommonJS-only package, you will get an error if you try to import it with import { RRule } from 'rrule' in an ESM project:

node index.mjs
file:///Users/k/p/ical-move-events/index.mjs:1
import { RRule } from 'rrule';
         ^^^^^
SyntaxError: Named export 'RRule' not found. The requested module 'rrule' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'rrule';
const { RRule } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:179:5)
    at async Loader.import (node:internal/modules/esm/loader:178:24)
    at async Object.loadESM (node:internal/process/esm_loader:68:5)
    at async handleMainPromise (node:internal/modules/run_main:63:12)

Unfortunately, this means that you still need to do unusual import syntax to make it work:

import pkg from 'rrule';
const { RRule } = pkg;

This issue with rrule not being published as real ESM is tracked over here: #512

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.

TypeScript + ESM
3 participants