feat: add `correct-ts-specifiers` by JakobJingleheimer · Pull Request #7 · nodejs/userland-migrations

JakobJingleheimer · 2024-12-01T00:04:41Z

Depends on #6

recipes/correct-ts-specifiers/.gitignore

AugustinMauroy

Okay frist review. mostly code style

recipes/correct-ts-specifiers/README.md

recipes/correct-ts-specifiers/package.json

recipes/correct-ts-specifiers/src/exts.ts

AugustinMauroy · 2024-12-02T08:34:32Z

recipes/correct-ts-specifiers/src/fexists.test.ts

+import assert from 'node:assert/strict';
+import { type Mock, afterEach, before, describe, it, mock } from 'node:test';
+import { fileURLToPath } from 'node:url';


Suggested change

import assert from 'node:assert/strict';

import { type Mock, afterEach, before, describe, it, mock } from 'node:test';

import { fileURLToPath } from 'node:url';

import assert from 'node:assert/strict';

import { afterEach, before, describe, it, mock } from 'node:test';

import { fileURLToPath } from 'node:url';

import type { Mock } from 'node:test'

I recommend separating js import and type import

Not opposed, but why?

but why?

So it's a bit stupid, but on the one hand you have an import of code (js) that will be executed and on the other hand the typing (ts/d.ts) that will be removed by the transpiler.

Hmm, I think combining them is better because it's easier to grok and realise A and type B are coming from the same place. Also, separating them rather significantly increases size.

That's true. I think they're both equally good. We'll have to see what the rest of the team thinks.

recipes/correct-ts-specifiers/src/is-ignorable-specifier.ts

recipes/correct-ts-specifiers/src/map-imports.test.ts

AugustinMauroy · 2024-12-02T08:42:14Z

recipes/correct-ts-specifiers/src/replace-js-ext-with-ts-ext.test.ts

+import assert from 'node:assert/strict';
+import path from 'node:path';
+import { type Mock, after, afterEach, before, describe, it, mock } from 'node:test';
+import { fileURLToPath } from 'node:url';


Suggested change

import assert from 'node:assert/strict';

import path from 'node:path';

import { type Mock, after, afterEach, before, describe, it, mock } from 'node:test';

import { fileURLToPath } from 'node:url';

import assert from 'node:assert/strict';

import path from 'node:path';

import { after, afterEach, before, describe, it, mock } from 'node:test';

import { fileURLToPath } from 'node:url';

import type { Mock } from 'node:test';

AugustinMauroy · 2024-12-02T08:43:31Z

recipes/correct-ts-specifiers/src/replace-js-ext-with-ts-ext.ts

+import type { FSAbsolutePath, NodeModSpecifier, ResolvedSpecifier, Specifier } from './index.d.ts';
+import { type DExt, type JSExt, type TSExt, extSets, suspectExts } from './exts.ts';
+import { fexists } from './fexists.ts';
+import { logger } from './logger.ts';
+import { isDir } from './is-dir.ts';


Suggested change

import type { FSAbsolutePath, NodeModSpecifier, ResolvedSpecifier, Specifier } from './index.d.ts';

import { type DExt, type JSExt, type TSExt, extSets, suspectExts } from './exts.ts';

import { fexists } from './fexists.ts';

import { logger } from './logger.ts';

import { isDir } from './is-dir.ts';

import type { FSAbsolutePath, NodeModSpecifier, ResolvedSpecifier, Specifier } from './index.d.ts';

import type { DExt, JSExt, TSExt } from './exts.ts';

import { extSets, suspectExts } from './exts.ts';

import { fexists } from './fexists.ts';

import { logger } from './logger.ts';

import { isDir } from './is-dir.ts';

AugustinMauroy · 2024-12-02T08:44:40Z

recipes/correct-ts-specifiers/src/workflow.ts

@@ -0,0 +1,56 @@
+import { type Api, api } from '@codemod.com/workflow';


Suggested change

import { type Api, api } from '@codemod.com/workflow';

import { api } from '@codemod.com/workflow';

import type { Api } from '@codemod.com/workflow';

Copilot

Copilot reviewed 76 out of 79 changed files in this pull request and generated no suggestions.

Files not reviewed (3)

recipes/correct-ts-specifiers/package.json: Language not supported
recipes/correct-ts-specifiers/src/fixtures/ambiguous.ts: Evaluated as low risk
recipes/correct-ts-specifiers/src/fixtures/bar.js: Evaluated as low risk

Comments skipped due to low confidence (1)

recipes/correct-ts-specifiers/src/fexists.test.ts:120

[nitpick] The test description is duplicated. Consider renaming it to 'should return false for a relative specifier with query parameter'.

it('should return `false` for a relative specifier', async () => {

…fiers

AugustinMauroy · 2024-12-13T08:35:52Z

recipes/correct-ts-specifiers/src/workflow.ts

+import { mapImports } from './map-imports.ts';
+import type { FSAbsolutePath } from './index.d.ts';
+
+module.register('@nodejs-loaders/alias', import.meta.url);


you can just import register because it's the only api we use on this file

Suggested change

module.register('@nodejs-loaders/alias', import.meta.url);

register('@nodejs-loaders/alias', import.meta.url);

I thought of that. I went with module.register to make it more clear what is happening (lots of APIs have register).

Is there a perf improvement to this or is it just 4 characters fewer?

Is there a perf improvement to this or is it just 4 characters fewer?

normally importing whole api isn't a issue, except when it's a big JS module

I don't feel strongly, maybe 6/10. If you feel stronger, we can go with that. Otherwise, meh, let's just leave it.

I think we can leave it like that. And yes, I ask a lot of questions, but this is the first codemod we've done, so we have to lay the foundations.

…fiers

recipes/correct-ts-specifiers/package.json

AugustinMauroy · 2024-12-16T22:45:30Z

recipes/correct-ts-specifiers/src/logger.ts

+	let hasError = false;
+
+	for (const [sourceFile, fileLog] of logs.entries()) {
+		console.log('[Codemod: correct-ts-specifiers]:', sourceFile);


maybe use styleText to have something coooollll

MVP first 😜 been working on this thing since like June

Oh yeah, holly molly macaroni

feat: add correct-ts-specifiers

384674b

AugustinMauroy reviewed Dec 2, 2024

View reviewed changes

recipes/correct-ts-specifiers/.gitignore Outdated Show resolved Hide resolved

AugustinMauroy reviewed Dec 2, 2024

View reviewed changes

JakobJingleheimer mentioned this pull request Dec 3, 2024

cli: allow test coverage, module mocks, and snapshots in NODE_OPTIONS nodejs/node#56114

Closed

JakobJingleheimer added 4 commits December 7, 2024 22:23

chore(deps): update to @nodejs-loaders/alias

71b2e73

fix: logger snap file name

4b09553

chore(ci): update import to use project's customiser

9dd61b0

fix(test): e2e main fixture

5f4df17

JakobJingleheimer mentioned this pull request Dec 7, 2024

chore: improve setup & CI config #6

Merged

JakobJingleheimer added 7 commits December 8, 2024 12:25

fixup!: correct types

cdedddd

chore(lint): de-lint

d39dc75

Merge branch 'main' into feat/correct-ts-specifiers

ccf9394

fixup!: remove redundant gitignore

21db5dd

fixup!: remove obsolete test badge from README

b977182

fixup!: tidy

bea0b23

fixup!: comment to explain why to _not_ ignore node_modules fixture

1e8ec00

JakobJingleheimer marked this pull request as ready for review December 11, 2024 10:29

AugustinMauroy requested review from a team and Copilot December 11, 2024 10:33

Copilot AI reviewed Dec 11, 2024

View reviewed changes

JakobJingleheimer force-pushed the feat/correct-ts-specifiers branch from 9da4e56 to 1e8ec00 Compare December 11, 2024 10:43

JakobJingleheimer added 3 commits December 11, 2024 11:43

Merge remote-tracking branch 'origin/main' into feat/correct-ts-speci…

79028f7

…fiers

fixup!: type for extensions

2b186c8

Merge remote-tracking branch 'origin/main' into feat/correct-ts-speci…

87c6a1f

…fiers

JakobJingleheimer force-pushed the feat/correct-ts-specifiers branch from 65c9502 to 87c6a1f Compare December 12, 2024 21:26

JakobJingleheimer added 3 commits December 12, 2024 23:39

fixup!: revert committing migration to e2e test fixture

b702093

fixup!: register @nodejs-loaders/alias within workflow

cdc5c41

fixup!: specifically mention monorepos in README

9dab933

AugustinMauroy reviewed Dec 13, 2024

View reviewed changes

fixup!: remove obsolete CLI command from run instructions

9824fd4

Merge remote-tracking branch 'origin/main' into feat/correct-ts-speci…

c1d9f99

…fiers

AugustinMauroy reviewed Dec 16, 2024

View reviewed changes

recipes/correct-ts-specifiers/package.json Outdated Show resolved Hide resolved

fixup!: update pjson github links

4fc0083

JakobJingleheimer force-pushed the feat/correct-ts-specifiers branch from 6fcffc0 to 037e5e0 Compare December 16, 2024 22:21

AugustinMauroy reviewed Dec 16, 2024

View reviewed changes

fixup!: add codemod package info

3d93029

JakobJingleheimer force-pushed the feat/correct-ts-specifiers branch from 037e5e0 to 3d93029 Compare December 18, 2024 19:42

JakobJingleheimer merged commit 33bb235 into main Dec 20, 2024

JakobJingleheimer deleted the feat/correct-ts-specifiers branch December 20, 2024 22:41

		@@ -0,0 +1,56 @@
		import { type Api, api } from '@codemod.com/workflow';

	import { type Api, api } from '@codemod.com/workflow';
	import { api } from '@codemod.com/workflow';
	import type { Api } from '@codemod.com/workflow';

	module.register('@nodejs-loaders/alias', import.meta.url);
	register('@nodejs-loaders/alias', import.meta.url);

Uh oh!

Conversation

JakobJingleheimer commented Dec 1, 2024

Uh oh!

Uh oh!

AugustinMauroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AugustinMauroy Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AugustinMauroy Dec 13, 2024 •

edited

Loading