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

Default import still broken with ESM and typescript moduleResolution node16 #61

Closed
apottere opened this issue Apr 30, 2024 · 9 comments
Closed

Comments

@apottere
Copy link

#59 was closed as complete, but the issue remains in the latest release version and also in trunk.

https://arethetypeswrong.github.io/?p=short-unique-id%405.1.1

With the newly added specs, I was able to confirm the error shown on the page above by making the following changes to the specs directory:

  • Add specs/package.json
    {
        "type": "module"
    }
  • Edit specs/import-esm.spec.ts to include .js in the import:
    import ShortUniqueId from '../dist/short-unique-id.js';
    

Then, running npx tsc in that directory produces the following error:

import-esm.spec.ts:3:18 - error TS2351: This expression is not constructable.
  Type 'typeof import("/[redacted]/short-unique-id/dist/short-unique-id")' has no construct signatures.

3 const suid = new ShortUniqueId({length: 3});
                   ~~~~~~~~~~~~~


Found 1 error in import-esm.spec.ts:3

I also confirmed that my project still has trouble building with the latest release version ([email protected])

@jeanlescure
Copy link
Collaborator

Thank you for the reproduction steps @apottere !

I'll take a look at this shortly 👍🏼

@jeanlescure
Copy link
Collaborator

The error gets fixed by Just moving the short-unique-id.js file to the specs directory and changing the import to:

import ShortUniqueId from './short-unique-id.js';

So this tells me the error from the above steps has more to do with module or directory context.

But, @apottere , since you mention you get an error when building your project, if you could provide an example project with the build tooling and steps you use, that would be most helpful.

In the mean-time I'll create a dummy project on my end to try and reproduce the error based on the info given so far, but given the symptoms, it's looking more and more like an edge case related to a specific set of tools and configurations, specially given the broad range of projects I've been successfully using short-unique-id on as well as projects successfully depending on it 🤔

Cheers

@jeanlescure
Copy link
Collaborator

I've been able to reproduce the error:

https://github.com/jeanlescure/suid-not-constructable-repro

After trying most combinations of config, it seems the only variable(s) affecting this is the usage of Node16 on module and/or moduleResolution on tsconfig.json.

Given that Node 16 reached end-of-life on September 11th, 2023, I'll just add a note on the README about the workaround:

import ShortUniqueId from 'short-unique-id';

const suid = new ShortUniqueId.default({length: 3}); // add .default

console.log(suid.rnd());

@apottere
Copy link
Author

Node 16 is EOL, but the node16 (aka nodenext) typescript module resolution is the most current and recommended setting. It's not the module resolution for node 16, it's the module resolution introduced in node 16. It's a terrible name for the setting, and has caused confusion before.

Docs for reference:

Because node16 and nodenext are the only module options that reflect the complexities of Node.js’s dual module system, they are the only correct module options for all apps and libraries that are intended to run in Node.js v12 or later, whether they use ES modules or not.

The only other modern option would be bundler, but that has other implications.

@apottere
Copy link
Author

apottere commented Apr 30, 2024

And a note about the workaround: While that allows typescript to compile, arethetypeswrong claims:

but that will likely fail at runtime

If you want a workaround that definitely works, here's what I'm using:

import ShortUniqueId from 'short-unique-id';

const shortCodeGenerator = new (ShortUniqueId as unknown as typeof ShortUniqueId.default)({
    // ...
});

This uses the constructor from the default export as intended, but appeases typescript by telling it that typeof ShortUniqueId.default is the type for that constructor.

@jeanlescure
Copy link
Collaborator

I just tried NodeNext again, you're right @apottere , I do get the error as well. I guess I didn't wait long enough before for the change to display the error. And I do agree it's a terrible name for the setting.

Ok, back to the drawing board in finding a possible fix from our end, but this is definitely a false type error detection coming from upstream via typescript; undoubtedly proven by the example you just provided 👏🏼

@apottere
Copy link
Author

After digging into it some more, I don't think there's a way to get a single file that retains what seems to be the desired behavior of the javascript (IIFE w/ esbuild) and working types with moduleResolution: node16. It seems like there are two options:

  1. Generate a new module-compatible javascript file with tsc and use conditional exports to support both esm and commonjs. This can be finicky, but is probably the proper solution.
  2. Change src/index.ts to export ShortUniqueId as a named export as well as the default export. That won't fix the current issue, but it would allow the workaround to be much simpler:
    // still broken
    import ShortUniqueId from 'short-unique-id';
    
    // works
    import { ShortUniqueId } from 'short-unique-id';

@jeanlescure
Copy link
Collaborator

@andrewbranch posted the fix on this issue:

ajv-validator/ajv#2132 (comment)

It was just a matter of declaring the namespace appropriately in the .d.ts:

import type {
    default as ShortUniqueIdCore,
    ShortUniqueIdRanges,
    ShortUniqueIdRangesMap,
    ShortUniqueIdDefaultDictionaries,
    ShortUniqueIdOptions,
    DEFAULT_UUID_LENGTH,
    DEFAULT_OPTIONS
} from './short-unique-id-core.d.ts';
declare namespace ShortUniqueId {
    export {
        ShortUniqueIdRanges,
        ShortUniqueIdRangesMap,
        ShortUniqueIdDefaultDictionaries,
        ShortUniqueIdOptions,
        DEFAULT_UUID_LENGTH,
        DEFAULT_OPTIONS
    }
}
declare class ShortUniqueId extends ShortUniqueIdCore {}
export = ShortUniqueId;

The export = ShortUniqueId; is the important snippet to fix the type resolution with Node16 / NodeNext.

I just released v5.2.0 which includes the fix. Let us know if this lets you get rid of the workaround in your project @apottere .

Cheers 🥂

@apottere
Copy link
Author

Oh perfect, I saw namespaces as a potential solution but didn't look into it. Looks like 5.2.0 is working in my project!

Thanks for the fix 🥂

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

No branches or pull requests

2 participants