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

Support module resolution for package.json exports #122

Merged
merged 1 commit into from
Feb 24, 2023
Merged

Support module resolution for package.json exports #122

merged 1 commit into from
Feb 24, 2023

Conversation

roydukkey
Copy link
Contributor

@slorber @Josh-Cena I've noticed that there are packages (e.g. @docusaurus/plugin-content-docs) that are using the NPM exports property. However, TypeScript isn't configured in a way that allows access to these items.

Given that Node 16.14 is required since 2.0.0-beta.21, I suggest that we can now add "moduleResolution": "Node16".

This allows the compiler and IDE to gather the proper information for usings the items in exports.

@Josh-Cena
Copy link
Contributor

Josh-Cena commented Jul 21, 2022

Reference: #107

I don't have strong opinions about using Node16 by default, as long as @orta thinks it's fine—I'm just worried not everyone is on 4.7 yet, considering there are so many changes in 4.7. Nevertheless, it always works if you add this in your local tsconfig.

@orta
Copy link
Member

orta commented Jul 21, 2022

I'm open to the docusaurus opinion to not be what I think about the default one for node: #121

But I generally think defaulting to ESM is probably going to surprise more folks than delight for a while

@Josh-Cena
Copy link
Contributor

I agree. For one thing—it often complicates things if you want to import packages with "type": "module". We already face this in the Docusaurus codebase and we have to slap //@ts-expect-error. I'm really looking forward to Webpack resolution in tsc.

@slorber
Copy link
Contributor

slorber commented Oct 19, 2022

FYI the init template of Docusaurus uses TS 4.7, and a user swizzling components will likely be exposed to APIs being declared in "exports".

It will most likely happen when swizzling "unsafe" theme components importing internal apis:

import {xyz} from '@docusaurus/theme-common/internal';

I can understand that there may be surprising behaviors (in which case the user can revert to "node") but think that this use-case above should rather work automatically by default. Maybe we can give it a try and see if anyone complains.

See related Docusaurus issue: facebook/docusaurus#8226


Another thing: I'm wondering if this default TS config shouldn't be versioned alongside Docusaurus. Will it always be reasonable that a default config would work fine for both Docusaurus 2.0 and future major versions?

If we have to ensure this package keeps being compatible with older Docusaurus sites, wouldn't this create friction because we can't enable new defaults on newly initialized sites because it breaks old sites?

Currently "@tsconfig/docusaurus": "^1.0.5",, shouldn't we apply a lower/upper range to this dependency and try to align with Docusaurus versions?

Relying on an external dependency with a different versioning scheme is likely to create unexpected breaking changes in the future.

What if we want to apply the change in this PR, but only for the upcoming Docusaurus v3.0 sites, and not current Docusaurus v2.0 sites?

@orta
Copy link
Member

orta commented Oct 19, 2022

You can apply semer to these bases also, so a bump to "@tsconfig/docusaurus": "^2.0.0" would be fine In your version v3 sites (as would a jump to v3) - but yeah, you can also totally take control here and ship a @docusaurus/tsconfig with your app!

That's also a good idea

@slorber
Copy link
Contributor

slorber commented Oct 20, 2022

If we keep config in this repo, that would be less confusion to have the same major version IMHO (ie Docusaurus v2 uses config v2)

Wonder what other frameworks do. Is nextjs extending the next config of this repo? Becaues I don't see a lot of npm downloads

@mamiu
Copy link

mamiu commented Feb 18, 2023

@orta are there any plans to merge this PR?

It took me more than an hour to find the fix described by @slorber.

@orta
Copy link
Member

orta commented Feb 18, 2023

It doesn't seem clear to me that a merge was what we wanted here? If it is, that's fine

@slorber
Copy link
Contributor

slorber commented Feb 24, 2023

@orta I'll probably look into reinterning this config later, but in the meantime, I think it's better to merge this pr and release it thanks ;)

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.

5 participants