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

Add subpath entry points + CommonJS types fix #503

Closed

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Mar 1, 2023

Hello 👋

We're using JSDoc types in CommonJS and looked like the types "require" entry got left behind recently?

I've added a fix and included a few other things:

CommonJS types

The types "require" entry doesn't export types GlobOptions, Result, Results etc

       "require": {
-        "types": "./dist/cjs/index-cjs.d.ts",
+        "types": "./dist/cjs/index.d.ts",
         "default": "./dist/cjs/index.js"
       }

This fixes type declarations in CommonJS such as:

/**
 * @param {import('glob').GlobOptions} [options] - Glob options
 */

Subpath package exports

I've added both "./*.js" (extensionless) and "./*" subpath exports to enable:

ES modules

import { hasMagic } from 'glob/has-magic.js'

CommonJS modules

const { hasMagic } = require('glob/has-magic')

But also in JSDoc comments:

/**
 * @param {import('glob/pattern').GlobList} globList - Glob list
 */

Declaration maps

The compiler option { "declarationMap": true } lets "Go to definition" jump to the TypeScript source

@isaacs
Copy link
Owner

isaacs commented Mar 1, 2023

I'd personally rather not expose the internal structure of the library by exporting all the *.js files. But yes, the type should be index.d.ts rather than index-cjs.d.ts, it seems.

@isaacs
Copy link
Owner

isaacs commented Mar 1, 2023

Oh, snap, I accidentally changed the export to index.js, and left the type as index-cjs.d.ts. That's 100% wrong, lol.

isaacs added a commit that referenced this pull request Mar 1, 2023
I'd meant to update the types to point to index.d.ts, and inadvertently
changed the *export* to index.js instead, whoops.

Re: #503
@isaacs
Copy link
Owner

isaacs commented Mar 1, 2023

Just to clarify why I don't want to export the internal modules, it makes things a lot less flexible when there's a need to move things into a submodule, but still re-export it. As a very recent example, I just in 9.1.0 started to add escape/unescape to Glob, and then realized it would also be valuable in Minimatch. So if I'd supported import { escape } from 'glob/escape', then I would have had to have a src/escape.ts that just contains export { escape } from 'minimatch' hanging around forever. Easier to just leave it as import { escape } from 'glob', imo, and then crafting the export only happens in one place.

I realize there are ways to do similar things in package.json exports, but it's a place I never think to look, so I prefer to mostly just treat package.json as npm's property, and not touch it with my meat fingers, because as evidenced by this bug, I can't be trusted to do that correctly 😂

@isaacs
Copy link
Owner

isaacs commented Mar 1, 2023

Pulled in the declarationMap (TIL, adding to my project init scripts, thanks!) and the fixed export/types for CommonJS.

Going to close this, but happy to reopen if there's another reason I'm missing that would be a strong argument to allow submodule exports.

@isaacs isaacs closed this Mar 1, 2023
@colinrotherham
Copy link
Contributor Author

@isaacs Brilliant, appreciate the quick types fix and glad the declarationMap was useful

Don't worry, happy to help 😊

Yeah that makes total sense to prevent access. No strong argument in particular, just a few unreachable types in JSDoc where TypeScript utilities like ReturnType<Type> or Type['property'] can't dig them out.

It's nice not to break Node.js require.resolve('package-name/package.json') but that's another subpath anyway!

@isaacs
Copy link
Owner

isaacs commented Mar 1, 2023

Ah, ok. I'm happy to export those from the main module, if there's something that would be useful. I agree, it's a pita to have to do a ton of Awaited<ReturnType<(typeof obj)['method']>> type stuff.

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.

2 participants