Skip to content

[dev-2.0] Fix optional and rest parameters in TypeScript class method declarations #7863

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: dev-2.0
Choose a base branch
from
Open

[dev-2.0] Fix optional and rest parameters in TypeScript class method declarations #7863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d5985f
[dev-2.0] Fix optional and rest parameters in TypeScript class method…
SoundOfScooting Jun 2, 2025
1b7e808
[dev-2.0] Fix `@chainable` in class method declarations
SoundOfScooting Jun 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,14 @@
"license": "LGPL-2.1",
"browser": "./lib/p5.min.js",
"exports": {
".": "./dist/app.js",
"./core": "./dist/core/main.js",
".": {
Copy link
Contributor

Choose a reason for hiding this comment

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

@limzykenneth I think you updated type exporting in package.json last, does this look OK to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be neccessary the update all the exports fields and additionally add the field "type": "module" to package.json. This affects the esm build not the iife. By using the exports condition default the files will be interpreted as .cjs files which might also result in conflicts with the type definitions. I'll doublecheck later
see

Copy link
Author

Choose a reason for hiding this comment

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

The link you posted says default "Can be a CommonJS or ES module file".

Also, forgot to mention this here in the PR, but the other index.js exports fields don't have corresponding index.d.ts declaration files to point to. I think it's because those files don't have any doc comments themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this should be part of a separate issue.

Please correct me if I'm wrong because I'm still in the process of wrapping my head around generate-types.mjs and reading relevant issues.

I'm using publint and arethetypeswrong as a reference. Generally these tools are made to check node.js packages therefore only the bundler module resolution should be of interest. Nonetheless there are some resolution errors within the declaration files. E.g.:

  • p5.d.ts references itself /// <reference path="./p5.d.ts" />
  • p5.Color.d.ts imports itself import { Color } from '../color/p5.Color'; (There are similar issues in other .d.ts files) I assume it's due to circular dependencies (as stated in the rollup build logs)

Regarding the conditional exports:

  • "./friendlyErrors": "./dist/core/friendlyErrors/index.js", can't be resolved. The correct file path would be "./dist/core/friendly_errors/index.js"

Yes the link I posted refers to node's module resolution algorithm whereas the majority of the p5.js endusers and consumers will or should use a bundler to resolve modules. So the main take away of this link is:

Writing ES module syntax in "ambiguous" files incurs a performance cost, and therefore it is encouraged that authors be explicit wherever possible. In particular, package authors should always include the "type" field in their package.json files, even in packages where all sources are CommonJS. Being explicit about the type of the package will future-proof the package in case the default type of Node.js ever changes, and it will also make things easier for build tools and loaders to determine how the files in the package should be interpreted.

https://nodejs.org/api/packages.html#introduction_1

Copy link
Author

@SoundOfScooting SoundOfScooting Jun 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, it probably makes more sense as a separate issue then. I thought the problem would only require a small change so I was hoping to get it merged with the overload fix at the same time. If requested I will remove the change from this PR for a future one.

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing for the other exports we don't have a setup for their types just yet that would make sense? We don't need it now as the full ESM modular use is not at a stage where I want to push it publicly yet, but we can start thinking about how it would work.

I'm fully in favor of including "type": "module" in package.json and shift any remaining code we have that still uses cjs to ESM, I think dependencies wise it should all support ESM without issue now.

"types": "./types/p5.d.ts",
"default": "./dist/app.js"
},
"./core": {
"types": "./types/core/main.d.ts",
"default": "./dist/core/main.js"
},
"./shape": "./dist/shape/index.js",
"./accessibility": "./dist/accessibility/index.js",
"./friendlyErrors": "./dist/core/friendlyErrors/index.js",
Expand Down
16 changes: 11 additions & 5 deletions utils/helper.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,8 @@ function generateDeclarationFile(items, organizedData) {
params: (entry.params || []).map(param => ({
name: param.name,
type: generateTypeFromTag(param),
optional: param.type?.type === 'OptionalType'
optional: param.type?.type === 'OptionalType',
rest: param.type?.type === 'RestType'
})),
module,
submodule,
Expand All @@ -307,9 +308,14 @@ function generateDeclarationFile(items, organizedData) {
params: (entry.params || []).map(param => ({
name: param.name,
type: generateTypeFromTag(param),
optional: param.type?.type === 'OptionalType'
optional: param.type?.type === 'OptionalType',
rest: param.type?.type === 'RestType'
})),
returnType: entry.returns?.[0] ? generateTypeFromTag(entry.returns[0]) : 'void',
returnType: entry.tags?.find(tag => tag.title === "chainable")
? "this"
: entry.returns?.[0]
? generateTypeFromTag(entry.returns[0])
: 'void',
module,
submodule,
class: className,
Expand Down Expand Up @@ -423,7 +429,7 @@ export function generateTypeFromTag(param) {

let type = param.type;
let prefix = '';
const isOptional = param.type?.type === 'OptionalType';
const isOptional = param.optional || param.type?.type === 'OptionalType';
if (typeof type === 'string') {
type = normalizeTypeName(type);
} else if (param.type?.type) {
Expand All @@ -432,7 +438,7 @@ export function generateTypeFromTag(param) {
type = 'any';
}

if (param.type?.type === 'RestType') {
if (param.rest || param.type?.type === 'RestType') {
prefix = '...';
}

Expand Down
Loading