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

Improve jsdoc inside, generate .d.ts typings #490

Closed
JounQin opened this issue Jun 30, 2022 · 12 comments · Fixed by #508
Closed

Improve jsdoc inside, generate .d.ts typings #490

JounQin opened this issue Jun 30, 2022 · 12 comments · Fixed by #508

Comments

@JounQin
Copy link
Member

JounQin commented Jun 30, 2022

No description provided.

@BPScott
Copy link
Member

BPScott commented Jun 30, 2022

(Heya, @JounQin, I adore your recent improvements to the repo, thank you for taking over - Identifying the double resolveConfig was particularly cool!)

maybe use ts as source

I don't think this has much value. Recently I've gotten really good mileage out of using plain JS with JSDoc comments - this code can be type-checked with tsc so you get the value of having type checking and inference in your editor, but you don't need deal with any kind of transpilation

Prepare for ESM

eslint/eslint#15453 suggests that eslint doesn't currently support using plugins authored in mjs. Even when it does get supported, the conversion would be "the one require call becomes an import, and module.exports = becomes export default`, I don't think this will radically improve the readability of the codebase.

@JounQin
Copy link
Member Author

JounQin commented Jun 30, 2022

@BPScott

Yeah, // @ts-check should be OK for type checking

And I'm posting for comments, thanks.

I was thinking to release as dual package for cjs and esm at the same time.

"the one require call becomes an import, and module.exports = becomes export default`

If I'm going on, I would make sure to use rollup for transpiling export default to module.exports = , so nothing would be broken.

More and more tools are going to be EMS-only, I'm trying to follow.

@BPScott
Copy link
Member

BPScott commented Jun 30, 2022

I was thinking to release as dual package for cjs and esm at the same time.

I'm trying to work out what the gain of this would be - eslint only works in a node environment - commonjs files will always be supported.

I can't think of any case where a consumer would need to use an mjs version. At this moment in time eslint plugins authored in mjs are unsupported and in the world where they do become supported it seems like it would be fine to just ship an mjs file - at that point there'd be no need for shipping a cjs file too.

I think introducing a bundler and producing cjs and mjs outputs right now is overhead for no material gain. Once eslint supports plugins written in esm format, I'd support us switching from distributing a commonjs file to only distributing a esmodules file, but in both the present and future worlds I can't see a reason why we'd need to distribute both commonjs and esmodules files at the same time.

More and more tools are going to be ESM-only, I'm trying to follow.

I like this idea, and I support it, but I think we should time it so we make the switch to author and distribute (without the need of a build tool) an esmodules file at the point eslint supports plugins authored in the esmodules format.

@JounQin
Copy link
Member Author

JounQin commented Jun 30, 2022

ESLint will support ESM in the future, but I don't think they would enforce the users to use mjs, so cjs could still be used on the users' sides.

This issue if preparing for ESM, so I'd like to support them both at the same time.

@BPScott
Copy link
Member

BPScott commented Jun 30, 2022

I don't think they would enforce the users to use mjs, so cjs could still be used on the users' sides.

If a consumer does either const prettierPlugin = require('eslint-plugin-prettier') in a cjs file or import prettierPlugin from 'eslint-plugin-prettier' in an mjs file, then they shall both look at the commonjs file referenced in eslint-plugin-prettier's package.json main key, and load the content from it successfully.

We don't need to make any changes to the package to acomodate those use cases - they already work.

@JounQin
Copy link
Member Author

JounQin commented Jul 1, 2022

Yeah, but I still want a pure ESM entry, 😅

Of course it maybe related to personal taste now.

@JounQin
Copy link
Member Author

JounQin commented Aug 13, 2022

OK, now I thought TS + pure ESM may be overkill for this project, I'll improve jsdoc and generate .d.ts instead. HDYT?

@JounQin JounQin changed the title Prepare for ESM, maybe use ts as source Improve jsdoc inside, generate .d.ts typings Aug 13, 2022
@BPScott
Copy link
Member

BPScott commented Aug 18, 2022

Improving jsdoc seems reasonable. I don't think there's much value in generating/publishing a index.d.ts file though because consumers of eslint plugins don't import the plugin using require() in their eslintrc file (though there is a proposal in progress) - it's all done within eslint from a magic string (e.g. plugins: ['eslint-plugin-prettier']), so there's no way for any type-checking software to perform type inference because it doesn't know what package gets imported. At what point would the types in that d.ts file be used by consumers?

@JounQin
Copy link
Member Author

JounQin commented Aug 18, 2022

@BPScott
Copy link
Member

BPScott commented Aug 19, 2022

Nice! Though it looks like the types for various plugins lie within that project rather than being something that would live in the eslint-plugin-prettier package.

@JounQin
Copy link
Member Author

JounQin commented Aug 19, 2022

Nice! Though it looks like the types for various plugins lie within that project rather than being something that would live in the eslint-plugin-prettier package.

Because most plugins don't have types. 😅

And that's what I want to "fix".

@JounQin
Copy link
Member Author

JounQin commented Sep 1, 2022

close via #509

@JounQin JounQin closed this as completed Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants