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

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

Open
fire332 opened this issue Mar 27, 2024 · 15 comments
Labels
enhancement New feature or request

Comments

@fire332
Copy link

fire332 commented Mar 27, 2024

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

  • OS: Windows 11
  • Browser: Chrome 125.0.6368.2 (Official Build) dev (64-bit)
  • Luxon version: 4.5.2
@fire332 fire332 added the bug Something isn't working label Mar 27, 2024
@tonysamperi
Copy link
Owner

tonysamperi commented Mar 27, 2024

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.

@tonysamperi tonysamperi added enhancement New feature or request and removed bug Something isn't working labels Mar 27, 2024
@fire332
Copy link
Author

fire332 commented Mar 27, 2024

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: remix-run/remix#9097

@tonysamperi
Copy link
Owner

Interesting

@tonysamperi
Copy link
Owner

Working on this finally! Rough times...

@tonysamperi
Copy link
Owner

tonysamperi commented May 26, 2024

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
Copy link
Owner

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

@tonysamperi
Copy link
Owner

Published v5.0.0

@fire332
Copy link
Author

fire332 commented May 27, 2024

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

Sure. Thanks for the update.

@fire332
Copy link
Author

fire332 commented May 27, 2024

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 tonysamperi reopened this May 27, 2024
@tonysamperi
Copy link
Owner

@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
Copy link
Author

fire332 commented May 27, 2024

@tonysamperi
Copy link
Owner

@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
Copy link
Owner

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

@tonysamperi tonysamperi reopened this Jun 5, 2024
@fire332
Copy link
Author

fire332 commented Jun 5, 2024

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
Copy link
Owner

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants