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

feat: support typescript configs using ts-node #320

Merged
merged 4 commits into from
Dec 29, 2021
Merged

feat: support typescript configs using ts-node #320

merged 4 commits into from
Dec 29, 2021

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Dec 26, 2021

This should be all that's required to support typescript configs. This handles supporting explicit configs - I'll do a follow-up PR to allow the default config to be picked up if in TS.

@AriPerkkio could you give me some pointers on how you'd like to test this?

Closes #318

@AriPerkkio
Copy link
Owner

how you'd like to test this?

For happy-path test I'd like to see new integration test. These tests create new configuration file and run production build using node-pty. https://github.com/AriPerkkio/eslint-remote-tester/blob/master/test/integration/integration.test.ts

Currently each integration test creates new configuration file:

const { name, cleanup } = createConfiguration(options, baseConfigPath);

const name = `./test/integration/integration.config-${idCounter++}.js`;

These files are Javascript CommonJS modules:

fs.writeFileSync(name, `module.exports=${configText}`, 'utf8');

I think we could pass a flag from integration.test.ts via function runProductionBuild which would indicate a TypeScript configuration file should be created.

+ if(someOptionIndicatingTS) {
+     fs.writeFileSync(`${name}.ts`, `export default ${configText}`, 'utf8');
+ } else { 
-     fs.writeFileSync(name, `module.exports=${configText}`, 'utf8');
+     fs.writeFileSync(`${name}.js`, `module.exports=${configText}`, 'utf8');
+ }

For error case testing I think the loadConfig function could be exported and tested in test/unit/config.test.ts.

@G-Rath G-Rath marked this pull request as ready for review December 27, 2021 21:32
lib/config/load.ts Outdated Show resolved Hide resolved
test/utils.ts Show resolved Hide resolved
test/integration/integration.test.ts Outdated Show resolved Hide resolved
@AriPerkkio
Copy link
Owner

Thanks, looks good! I'll do some more manual testing later before merging this. Also some documentation updates to README.md are required.

I just realized we'll need some changes to eslint-remote-tester-run-action as well. Currently it holds an internal eslint-remote-tester.config.js which is extended by users configuration. I think there it would be enough to check whether user passed a .ts config in action parameters, and create internal configuration as typescript. I'll need to think this through.

https://github.com/AriPerkkio/eslint-remote-tester-run-action/blob/223700ccc8394c4e17d54e0d9d5d43dba8fed06c/src/run-tester.ts#L29-L30

Copy link
Owner

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

Merging these functional changes as is, in order to keep PR small. Let's continue documentation and configuration type exporting at #321.

I'll do a follow-up PR to allow the default config to be picked up if in TS.

This can also be started now if you're up for it @G-Rath. Thanks!

@AriPerkkio AriPerkkio merged commit b5f1252 into AriPerkkio:master Dec 29, 2021
@AriPerkkio
Copy link
Owner

@G-Rath as part of ESLint v9 migration (#519) I'm converting the configuration loading to ESM. Any ideas how to register ts-node for ESM?

@AriPerkkio
Copy link
Owner

I think I'll switch it to https://github.com/antfu/importx instead.

@G-Rath
Copy link
Contributor Author

G-Rath commented May 29, 2024

@AriPerkkio it should be doable though I've not done it myself - importx looks cool, though it seems like it directly depends on all the loaders it supports, which doesn't seem ideal (they should be optional peer dependencies instead)

@AriPerkkio
Copy link
Owner

Good point, install size of importx is quite much.

Looks like ESLint itself has chosen jiti. https://github.com/eslint/eslint/pull/18134/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R172-R179
I'll switch ts-node to jiti so users can share the peer dependency on both projects.

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.

feat: support config file as typescript
2 participants