tonysamperi / ts-luxon

Typescript based Luxon: ⏱ A library for working with dates and times in TS and JS (immutable)
https://tonysamperi.github.io/ts-luxon
MIT License
13 stars 3 forks source link

Import issues in modern Node versions due to improperly declared package.json #11

Open fire332 opened 8 months ago

fire332 commented 8 months ago

Describe the bug

import { DateTime } from 'ts-luxon';
         ^^^^^^^^
SyntaxError: The requested module 'ts-luxon' does not provide an export named 'DateTime'

ts-luxon needs to have a properly declared exports field in package.json for Node to use the ESM bundle. Additionally, the exports need to have the correct file extension to avoid incorrect interpretation.

Example:

{
    "type": "commonjs",
    "exports": {
        ".": {
            "types": "./dist/index.d.ts",
            "import": "./dist/ts-luxon.es6.mjs", // NOTE: .mjs file extension
            "default": "./dist/ts-luxon.umd.js" // NOTE: interpreted as commonjs due to `type` field
        }
    }
}

Note that the example has this issue. See: https://publint.dev/ts-luxon@4.5.2

To Reproduce https://stackblitz.com/edit/stackblitz-starters-gpzsez?file=index.js

Actual vs Expected behavior No import errors.

Desktop (please complete the following information):

tonysamperi commented 8 months ago

Now this is a very useful issue! Thanks mate! So I could basically release a V5 to fix the package exports. I definitely will!

By the way it's not a bug because this library was mainly intended to work with Typescript and was supposed to work via umd with javascript.

fire332 commented 8 months ago

The bug label is auto-added by GitHub and I have no control over the labels.

I also do consider this a bug since with certain server-side rendering implementations, ts-luxon would be directly imported on the server and not processed through a bundler which would mask this issue.

I've actually originally filed this as a Remix bug here: https://github.com/remix-run/remix/issues/9097

tonysamperi commented 8 months ago

Interesting

tonysamperi commented 6 months ago

Working on this finally! Rough times...

tonysamperi commented 6 months ago

Published v 5.0.0-beta.1 (based on 4.6.0 that I published earlier, so we're aligned to the latest luxon developments that they haven't even published yet :D)

The problem would seem solved.

https://stackblitz.com/edit/stackblitz-starters-gulgmw?file=package.json,index.js

Let me do some tests in the afternoon to see if it also solves the "CommonJS" problem in my other application...

Then I can publish the official 5.0.0...but for now you can enjoy this beta.

Thanks again for the help.

tonysamperi commented 6 months ago

@fire332 can I quote you in the thanks section of the readme?

tonysamperi commented 6 months ago

Published v5.0.0

fire332 commented 6 months ago

@fire332 can I quote you in the thanks section of the readme?

Sure. Thanks for the update.

fire332 commented 6 months ago

Note that the fix in 5.0 still has the issue noted in the example where TS will allow a default import when it doesn't exist in the package.

https://publint.dev/ts-luxon@5.0.0 https://stackblitz.com/edit/stackblitz-starters-y4jgeo?file=index.ts

Note how there are no TS errors in the stackblitz example but Node will complain about the default export not existing.

Explanation of the issue and how to fix: https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md

tonysamperi commented 6 months ago

@fire332 ok since it worked in my scenario I needed to have this released. I actually noticed the "mts" suggestion in the publint.dev but I honestly don't know how to produce it. From the docs you linked it's not clear, from what I understand it just needs to be there. If contents are different, there's no mention.

Luckily I don't need another major for this... It could be a patch release..

Keep you posted.

PS I already published now that I have the green light I'll mention you on next publish 😉

fire332 commented 6 months ago

These links may be helpful:

https://github.com/microsoft/TypeScript/issues/54593 https://github.com/arethetypeswrong/arethetypeswrong.github.io/issues/21

tonysamperi commented 5 months ago

@fire332 man I can't do it. It's simply not possible, at least with rollup probably. I tried the following, but the result was always "Masquerade as CJS".

  1. Outputting to separate dist folders: produced duplicated typings
  2. Adding a second tsconfig and running a second typescript bundle
  3. Using babel along rollup (didn't even manage to compile, it compiled fine on it's own, but unbundled outputs)

Probably the way would be bundling with Webpack.

I'm closing this for now.

tonysamperi commented 5 months ago

Ok I think I get it. I should rely on CJS. I don't know why outputting esm modules should be so painful, but even after refactoring the entire build flow with Webpack, there's simply no way of achieving it. I'm about to release another beta... Then run a few tests and if stable I will publish 5.1.0 and deprecate all 5.0.x

fire332 commented 5 months ago

Mind if I take a crack at it?

EDIT: tbh, it's probably easier to just drop support for the CJS bundle as ts-luxon will either be used directly in Node.js which supports ES modules (even the oldest LTS version), or it'll be used with a bundler which will handle the ESM/CJS interop anyways.

tonysamperi commented 5 months ago

@fire332 I agree that ts-luxon is intended for interop, the point is that I tried everything, including removing the umd bundle and leave it a module...it always resulted in ESM with CJS typings.

If you want to give it a try you're welcome. I only ask you a favor, let's update in a couple of weeks from now (in the meantime you can fork the repo and play).

There's really something I need to get over with that depended on the refactor being stable.

But I'm open to bump directly to a pure ESM V6 if we find a way to make it working.

Thanks again for your input.