Migrate Levitate to ESM #423

academo · 2024-02-20T16:09:40Z

What this PR does / why we need it:

Moves Levitate from commonjs to a ESM library

Relevant changes

Execa updated to its latest esm version
Node-fetch updated to its latest esm version
Jest replaced by Vitest because of its native support for ESM
Levignore file reading now requires copying the file to a temporal file because of how esm imports cjs files

Which issue(s) this PR fixes:

Closes #393

Special notes for your reviewer:
Do not get scared by the file count, most of the changes are changing imports to have .js extension in them

academo · 2024-02-20T16:11:07Z

src/bin.ts

-import { getListImportsCliArgs, CliError } from './utils/cli';
-import { resolvePackage, resolveTargetPackages } from './utils/npm';
-import { getExportInfo } from './compiler/exports';
+import { getImportsInfo, getGroupedImports } from './compiler/imports.js';


This is the bulk of this PR. changing all these files to import with the .js extension

src/bin.ts

academo · 2024-02-20T16:12:27Z

src/print/index.ts

-export * from './removals';
-export * from './utils';
-export * from './veredict';
+export { printChanges } from './changes.js';


I removed the barrel exports from here. I wanted to do the same in the index file but there are so many exports in that file I'd have to spend a whole day working on that.

academo · 2024-02-20T16:14:00Z

src/utils.ts

@@ -40,11 +41,18 @@ export async function readLevignoreFile(cwdPath: string): Promise<IgnoreExportCh
      return {};
    }

-    const levignoreFileContent = require(levignoreFilePath);
+    const tempFile = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'levitate')), 'levignore.cjs');


This change is annoying but I can't find a better way to do it. the levignore.js file is a commonjs file but it has a .js extension. trying to await import(file) directly will throw a js error, and the import function doesn't have parameters to tell it it is a commonjs file, it seems to only go based on file extension, thus, I have to copy this in a temporal folder and read it from there.

I would probably add the gist of the explanation as a comment, so it's easy to remember later why we were doing it like this.

Good idea. I'll do that

Adding type: module to package.json instructs node to read all js files as esm. Are you trying to import this file?

@jackw yes. do you know a better alternative?

Would it work if the file you want to import was converted to an es module?

We could, but then we'd break compatibility with projects already using levignore.js in its cjs format.

academo · 2024-02-20T16:15:10Z

src/utils/npm.ts

@@ -187,7 +188,7 @@ export function getPackageJson(packagePath: string): any {
    return null;
  }

-  return require(getPackageJsonPath(packagePath));
+  return readJsonFile(getPackageJsonPath(packagePath));


simple change, since require is not defined in esm and we are simply loading a json file. I could had used module.createRequire but this is a simpler solution IMO

Don't we have await import(file, { assert: { type: 'json' } } support in node 20?

I want to keep compatibility with node 18 which is still LTS.

leventebalogh

LGTM 👍

leventebalogh · 2024-02-21T14:48:43Z

src/utils.ts

@@ -40,11 +41,18 @@ export async function readLevignoreFile(cwdPath: string): Promise<IgnoreExportCh
      return {};
    }

-    const levignoreFileContent = require(levignoreFilePath);
+    const tempFile = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'levitate')), 'levignore.cjs');


I would probably add the gist of the explanation as a comment, so it's easy to remember later why we were doing it like this.

jackw

Great work on this @academo ! 🚀

I've left a couple of comments regarding node support and json import.

src/bin.ts

src/compiler/exports.test.ts

jackw · 2024-02-21T15:52:57Z

src/utils.ts

@@ -40,11 +41,18 @@ export async function readLevignoreFile(cwdPath: string): Promise<IgnoreExportCh
      return {};
    }

-    const levignoreFileContent = require(levignoreFilePath);
+    const tempFile = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'levitate')), 'levignore.cjs');


Adding type: module to package.json instructs node to read all js files as esm. Are you trying to import this file?

jackw · 2024-02-21T15:55:16Z

src/utils/npm.ts

@@ -187,7 +188,7 @@ export function getPackageJson(packagePath: string): any {
    return null;
  }

-  return require(getPackageJsonPath(packagePath));
+  return readJsonFile(getPackageJsonPath(packagePath));


Don't we have await import(file, { assert: { type: 'json' } } support in node 20?

jackw · 2024-02-21T15:57:10Z

tsconfig.json

@@ -1,15 +1,25 @@
 {
  "$schema": "http://json.schemastore.org/tsconfig",
  "compilerOptions": {
-    "module": "commonjs",
-    "moduleResolution": "node",
+    "module": "node16",


In the future it would be good to ditch as much of this as we can favour of the base configs.

I am not a fan of the idea of having a third party determining our configuration and introducing breaking changes at will and breaking our projects

Not sure I agree with that. The breaking change in this example is related to Next. If you want to target node18 why don't we use the recommended node18 tsconfig as we're doing in plugin-tools?

I rather keep that change out of this PR. I opened an issue here #424 so we can do the change later.

academo added 9 commits February 20, 2024 11:18

WIP: Step 1 ESM convert

03714a3

Add exports

78644b7

Update configuration

68004db

Update all imports to use .js

4d89537

Fix barrel exports in print

1d87881

Update barrels from index

eb5421f

Fix typescript code

f5579ef

Replace jest with vitest

0b56521

Update dependencies

c62aa22

academo requested a review from a team as a code owner February 20, 2024 16:09

academo requested review from Ukochka, jackw and leventebalogh and removed request for a team February 20, 2024 16:09

academo commented Feb 20, 2024

View reviewed changes

src/bin.ts Show resolved Hide resolved

academo commented Feb 20, 2024

View reviewed changes

academo self-assigned this Feb 20, 2024

leventebalogh approved these changes Feb 21, 2024

View reviewed changes

jackw approved these changes Feb 21, 2024

View reviewed changes

academo added 3 commits February 22, 2024 09:35

Replace node20 APIs with node18 compatible ones

1c96541

Introduce test:ci beacuse vitest runs on watch by default

205f92d

Add clarifying comment for levignore imports

e440d98

academo merged commit 1dc8044 into main Feb 22, 2024
2 checks passed

academo deleted the academo/levitate-to-esm branch February 22, 2024 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Levitate to ESM #423

Migrate Levitate to ESM #423

academo commented Feb 20, 2024 •

edited

academo Feb 20, 2024

academo Feb 20, 2024

academo Feb 20, 2024

leventebalogh Feb 21, 2024

academo Feb 21, 2024

jackw Feb 21, 2024

academo Feb 22, 2024

jackw Feb 22, 2024 •

edited

academo Feb 22, 2024

academo Feb 20, 2024

jackw Feb 21, 2024 •

edited

academo Feb 22, 2024

leventebalogh left a comment

leventebalogh Feb 21, 2024

jackw left a comment

jackw Feb 21, 2024

jackw Feb 21, 2024 •

edited

jackw Feb 21, 2024

academo Feb 22, 2024

jackw Feb 22, 2024

academo Feb 22, 2024

Migrate Levitate to ESM #423

Migrate Levitate to ESM #423

Conversation

academo commented Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackw Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackw Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leventebalogh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackw Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

academo commented Feb 20, 2024 •

edited

jackw Feb 22, 2024 •

edited

jackw Feb 21, 2024 •

edited

jackw Feb 21, 2024 •

edited