-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[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.
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
base: dev-2.0
Are you sure you want to change the base?
[dev-2.0] Fix optional and rest parameters in TypeScript class method declarations #7863
Conversation
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SoundOfScooting, the type building changes look good to me! I'm just tagging another maintainer to take a look at the package.json
changes before we merge it in 🙂
@@ -68,8 +68,14 @@ | |||
"license": "LGPL-2.1", | |||
"browser": "./lib/p5.min.js", | |||
"exports": { | |||
".": "./dist/app.js", | |||
"./core": "./dist/core/main.js", | |||
".": { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Addresses #7862 (fixes overloads but not missing fields or other syntax errors)
Changes:
package.json
so they can be imported.Screenshots of the change:
PR Checklist
npm run lint
passes