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

fix(core): typings path and export #1708

Closed
wants to merge 1 commit into from

Conversation

sinedied
Copy link
Contributor

Typescript compiler doesn't seem to find the types unless the full path is specified.
And since the types path doesn't match the exports path, it needs to be added there as well

image

Typescript compiler doesn't seem to find the types unless the full path is specified
@sinedied sinedied marked this pull request as ready for review July 21, 2023 08:35
},
"require": {
"default": "./dist/node/asciidoctor.cjs",
"types": "./types/index.d.ts"
Copy link
Member

@ggrossetie ggrossetie Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we need to specify a d.cts types file (CommonJS). Since we are using export default function in index.d.ts I don't think we should specify types on require.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be right regarding the d.cts extension, seeing microsoft/TypeScript#52363
But if you look at the answers in the same thread, I think there's still need to specify the types explicitly on the require conditional, as there's not matching patch TypeScript will not load it.

My current project is configured for ESM so I could not test the CJS part, it would be helpful to check it on a real CJS project. I'll try to build a test project for that next monday

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks that would be helpful!
If it works on CJS we can proceed and merge.

@Techassi
Copy link

I'm running into the same error. Related issues might be #1711 and #1588.

@ggrossetie
Copy link
Member

@Techassi if you could confirm that the above configuration works in a CJS project that would be great.

@Techassi
Copy link

Techassi commented Nov 25, 2023

I also don't use CJS style imports, but I can create a small test project and try it out. Providing the correct path is essential, as currently the @asciidoctor/core package is unusable when using ESM style imports.

@Techassi
Copy link

Techassi commented Nov 28, 2023

I just tested it locally. This set of options in package.json seems to work:

{
  "exports": {
    "require": {
      "default": "./dist/node/asciidoctor.cjs",
      "types": "./types/index.d.ts"
    },
    "import": {
      "default": "./dist/node/asciidoctor.js",
      "types": "./types/index.d.ts"
    }
  },
  "types": "types/index.d.ts",
}

We might want to change ./types/index.d.ts to ./types/index.d.cts in the require group of options. I didn't look into generating this file (it should be a simple copy) as the build scripts seem quite convoluted.

Side note: It might be good to drop CJS style imports (and types) altogether, as the JS ecosystem is moving to the more modern ESM style. It was quite a hassle to set up this local test project as most modern tooling defaults to ESM.

@ggrossetie
Copy link
Member

I just tested it locally. This set of options in package.json seems to work:

OK, so even if the file index.d.ts contains ESM import and export keywords it's working on a CJS project?

We might want to change ./types/index.d.ts to ./types/index.d.cts in the require group of options. I didn't look into generating this file (it should be a simple copy) as the build scripts seem quite convoluted.

I found a rollup pluign: https://github.com/Swatinem/rollup-plugin-dts

Side note: It might be good to drop CJS style imports (and types) altogether, as the JS ecosystem is moving to the more modern ESM style. It was quite a hassle to set up this local test project as most modern tooling defaults to ESM.

We dropped CJS in the first release candidate of Asciidoctor.js but the community was vocal about keeping it. Asciidoctor.js code base is now using ESM and we are using rollup to produce CJS. We might remove CJS support in the next major release.

@ggrossetie
Copy link
Member

Superseded by #1715 (@sinedied I cherry-picked your commit)

@ggrossetie ggrossetie closed this Nov 29, 2023
@Techassi
Copy link

Techassi commented Nov 29, 2023

OK, so even if the file index.d.ts contains ESM import and export keywords it's working on a CJS project?

Yes seems like it. I got the correct typing information when using CJS imports and the above export options.

I found a rollup pluign: Swatinem/rollup-plugin-dts

Oh yeah, I actually use it in one of my personal projects as well. Had great success working with it. It might require some output path adjustments tho.

We might remove CJS support in the next major release.

I would love to see that!


Thanks for getting this merged super quick. Will test it in the real project asap!

@sinedied
Copy link
Contributor Author

Thanks @Techassi and @ggrossetie for picking up this and completing it 👍

@Techassi
Copy link

Do you have any plans on when this will be released @ggrossetie? Currently, I still use a local dependency as a workaround.

@ggrossetie
Copy link
Member

I will try to push a new release before my vacation (mid January)

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.

3 participants