jkbrzt / rrule

JavaScript library for working with recurrence rules for calendar dates as defined in the iCalendar RFC and more.
https://jkbrzt.github.io/rrule
Other
3.29k stars 514 forks source link

Switch from faux-ESM to real ESM exports #512

Closed karlhorky closed 2 years ago

karlhorky commented 2 years ago

Reporting an issue

Thank you for taking an interest in rrule! Please include the following in your report:

The "module" field in package.json is non-official, and not a real ESM specification of npm: https://stackoverflow.com/a/42817320/1268612

Switching away from "module" to use the "exports" and "type": "module" fields in the package.json would allow for better compatibility with ESM, everywhere it is used.

You may even consider making it a pure ESM package, as mentioned here: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

Expected output (see "type" and "exports" fields):

$ cat node_modules/rrule/package.json
{
  "name": "rrule",
  "version": "2.7.0",
  ...
  "main": "dist/es5/rrule.js",
  "type": "module",
  "exports": "./dist/esm/src/index.js",
  "types": "dist/esm/src/index.d.ts",
  ...
}

Actual output (see the outdated faux-ESM "module" field):

$ cat node_modules/rrule/package.json
{
  "name": "rrule",
  "version": "2.7.0",
  ...
  "main": "dist/es5/rrule.js",
  "module": "dist/esm/src/index.js",
  "types": "dist/esm/src/index.d.ts",
  ...
}
davidgoli commented 2 years ago

Thanks for the suggestion. I don't think we're ready to abandon cjs users at this point, especially considering how new ESM is to Node itself, but I would like to improve the ESM support. What do you think about a solution like the one outlined here: https://redfin.engineering/node-modules-at-war-why-commonjs-and-es-modules-cant-get-along-9617135eeca1

karlhorky commented 2 years ago

Maybe it would work, I've heard some (rare) accounts of projects providing both ESM and CJS successfully. Any kind of improvement would be great! 👍

davidgoli commented 2 years ago

Please take a look at the latest (2.7.1) and let me know how it's working for you now

karlhorky commented 2 years ago

@davidgoli thanks for the new version!

rrule is still not published as ESM.

Importing and using rrule in the following file (import copied from the rrule readme) leads to an error.

index.mjs

import { RRule } from 'rrule';

console.log(RRule);

The error is this below:

$ 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)

Luckily there is a workaround and it is printed out in the error message, but a lot of users will probably not notice it or not understand:

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

console.log(RRule);

So probably this issue should be reopened, for seamless real ESM support.

karlhorky commented 2 years ago

@davidgoli do you think you could reopen this issue? I think it is not resolved yet.

karlhorky commented 2 years ago

@davidgoli I've opened a new issue for this instead:

Superseded by https://github.com/jakubroztocil/rrule/issues/548

JuicyZest commented 9 months ago

Same Issue here, import pkg from 'rrule'; const { RRule } = pkg; This workaround resolves the module error, but gives me a RRule is not a constructor error when trying to create a new RRule object. Anyone ever found a solution ?

karlhorky commented 9 months ago

@JuicyZest this issue is closed, can you copy your comment above to the current issue over here?