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: add compatibility for modern module resolutions #157

Conversation

kyranet
Copy link
Contributor

@kyranet kyranet commented May 31, 2023

Defines the correct typings file in the types field, and adds the same field inside the exports for correct TypeScript module resolution support.

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR solves the type resolution for this package, previously it would fail and instead, TypeScript would try to load (or rather, infer) the types from the ./dist/module.mjs file rather than loading the types property in package.json, this is, after all, because the exports field overrides it.

This field is also order-sensitive, TypeScript will try to load the first condition on the object (the properties are, after all, conditions that are tested against in insertion order). This will not change the behavior when loading the package from Node.js, as types is not a valid condition for the runtime. You can see a PR with a detailed description here.

I have run yarn patch with this exact change and after it, Nuxt to properly find the type. This PR mirrors said patch. If replicating this issue comes out complicated, I believe you may need to set typescript.tsConfig.compilerOptions.moduleResolution to 'bundler' (alternatively node16 or nodenext) in your defineNuxtConfig so TypeScript loads the exports field rather than trying the top-level properties. This is tested with TypeScript v5 (the latest stable as of writing).

Without this PR, Nuxt cannot infer the type from the default export, making the security property be typed as Record<string, any> rather than ModuleOptions, as seen from the NuxtConfig declaration:

declare module 'nuxt/schema' {
	interface NuxtConfig {
		["security"]?: typeof import("nuxt-security").default extends NuxtModule<infer O> ? Partial<O> : Record<string, any>
		// ...
	}
	// ...
}

Importing the types from module.d.ts rather than types.d.ts was also a deliberate choice, as of 0.13.0, the types.d.ts file doesn't allow importing other types (such as ModuleOptions) as it restricts the exports to the default export, which may be undesirable:

import {  } from './module'
export { default } from './module'

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

Not applicable since it affects how module resolution works and does not change the library's behavior.

@vercel
Copy link

vercel bot commented May 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nuxt-security ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2023 8:26pm

@Baroshem
Copy link
Owner

Hey @kyranet

Sorry for no contact from my side. I was off for short holidays and then I completely missed this PR. I will review it and release it probably with the next release to unblock you :)

@Baroshem Baroshem merged commit ea6a417 into Baroshem:main Jun 19, 2023
3 checks passed
@kyranet kyranet deleted the fix/add-compatibility-for-modern-module-resolutions branch June 19, 2023 14:14
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.

None yet

2 participants