phenomnomnominal / betterer

betterer makes it easier to make incremental improvements to your codebase
MIT License
581 stars 39 forks source link

Betterer hangs in Angular 13 #936

Open railsstudent opened 2 years ago

railsstudent commented 2 years ago

Describe the bug Better hangs and cannot finish any test after Angular application is upgrade to 13 and it worked in Angular 12.

To Reproduce https://github.com/railsstudent/ng-spanish-menu/tree/upgrade-angular13

Expected behavior Before Angular upgrade, betterer runs all the tests and return the scores successfully

Screenshots If applicable, add screenshots to help explain your problem.

Versions (please complete the following information):

Debug log:

Error: could not import config from "/Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/.betterer.ts". 😔 at loadTestMetaFromConfig (/Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@betterer/betterer/src/test/loader.ts:30:11) at /Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@betterer/betterer/src/test/loader.ts:11:22 at Array.map () at loadTestMeta (/Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@betterer/betterer/src/test/loader.ts:10:22) at BettererContextΩ.run (/Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@betterer/betterer/src/context/context.ts:64:36) at async BettererContextΩ.runOnce (/Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@betterer/betterer/src/context/context.ts:85:5)

Error:Must use import to load ES Module: /Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@angular/compiler-cli/bundles/index.js require() of ES modules is not supported. require() of /Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@angular/compiler-cli/bundles/index.js from /Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@betterer/angular/dist/angular.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules. Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@angular/compiler-cli/package.json.

require() of ES modules is not supported. require() of /Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@angular/compiler-cli/bundles/index.js from /Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@betterer/angular/dist/angular.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules. Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/connieleung/Documents/ws_jsangular2/ng-spanish-menu/node_modules/@angular/compiler-cli/package.json.

  at new NodeError (internal/errors.js:322:7)
  at Object.Module._extensions..js (internal/modules/cjs/loader.js:1102:13)
  at Module.load (internal/modules/cjs/loader.js:950:32)
  at Function.Module._load (internal/modules/cjs/loader.js:790:12)
  at Module.require (internal/modules/cjs/loader.js:974:19)
  at require (internal/modules/cjs/helpers.js:93:18)

⸨⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⠂⸩ ⠧ : timing npm:load Completed in 91ms

Additional context Add any other context about the problem here.

michealharrington commented 2 years ago

I am getting a very similar error after upgrading to Angular 13.

Error: could not import config from "/home/mharrington/workspace/slam-ui/.betterer.ts". 😔
      at loadTestMetaFromConfig (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/src/test/loader.ts:30:11)
      at /home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/src/test/loader.ts:11:22
      at Array.map (<anonymous>)
      at loadTestMeta (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/src/test/loader.ts:10:22)
      at BettererContextΩ.run (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/src/context/context.ts:83:36)
      at async BettererContextΩ.runOnce (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/src/context/context.ts:111:7)
      at async betterer (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/src/betterer.ts:31:10)
      at async Command.<anonymous> (/home/mharrington/workspace/slam-ui/node_modules/@betterer/cli/src/start.ts:38:28)
      at async Command.parseAsync (/home/mharrington/workspace/slam-ui/node_modules/@betterer/cli/node_modules/commander/lib/command.js:923:5)
      at async Object.cli__ (/home/mharrington/workspace/slam-ui/node_modules/@betterer/cli/src/cli.ts:58:3)

Error: require() of ES Module /home/mharrington/workspace/slam-ui/node_modules/@angular/compiler-cli/bundles/index.js from /home/mharrington/workspace/slam-ui/node_modules/@betterer/angular/dist/angular.js not
       supported.
       Instead change the require of index.js in /home/mharrington/workspace/slam-ui/node_modules/@betterer/angular/dist/angular.js to a dynamic import() which is available in all CommonJS modules.
  Instead change the require of index.js in /home/mharrington/workspace/slam-ui/node_modules/@betterer/angular/dist/angular.js to a dynamic import() which is available in all CommonJS modules.
      at Object.require.extensions.<computed> [as .js] (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/node_modules/ts-node/dist/index.js:785:20)
      at Object.<anonymous> (/home/mharrington/workspace/slam-ui/node_modules/@betterer/angular/dist/angular.js:4:24)
      at Object.require.extensions.<computed> [as .js] (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/node_modules/ts-node/dist/index.js:785:20)
      at Object.<anonymous> (/home/mharrington/workspace/slam-ui/node_modules/@betterer/angular/dist/index.js:9:17)
      at Object.require.extensions.<computed> [as .js] (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/node_modules/ts-node/dist/index.js:785:20)
      at Object.<anonymous> (/home/mharrington/workspace/slam-ui/.betterer.ts:5:19)
      at Module.m._compile (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/node_modules/ts-node/dist/index.js:791:29)
      at Object.require.extensions.<computed> [as .ts] (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/node_modules/ts-node/dist/index.js:793:16)
      at requireUncached (/home/mharrington/workspace/slam-ui/node_modules/@betterer/betterer/dist/require.js:7:15)
michealharrington commented 2 years ago

Any updates? Where you able to find a workaround?

phenomnomnominal commented 2 years ago

So there isn't a workaround - it requires reworking quite a bit of Betterer code to work with ES Modules. That's something I want to do anyway but it's a little bit painful and slow, and I'm a bit time poor right now. Hope to have news soon!

michealharrington commented 2 years ago

Is there anything I can change to work around this issue?

michealharrington commented 2 years ago

Any updates on this? This is a blocker for me upgrading to Angular 13.

Is there a config change I can make to by pass this exception?

Error: could not import config from "/home/<>/git/slam-ui/.betterer.ts". 😔
      at loadTestMetaFromConfig (/home/<>/git/slam-ui/node_modules/@betterer/betterer/src/test/loader.ts:30:11)
      at /home/<>/git/slam-ui/node_modules/@betterer/betterer/src/test/loader.ts:11:22
      at Array.map (<anonymous>)
      at loadTestMeta (/home/<>/git/slam-ui/node_modules/@betterer/betterer/src/test/loader.ts:10:22)
      at BettererContextΩ.run (/home/<>/git/slam-ui/node_modules/@betterer/betterer/src/context/context.ts:83:36)
      at async BettererContextΩ.runOnce
  (/home/<>/git/slam-ui/node_modules/@betterer/betterer/src/context/context.ts:111:7)
      at async betterer (/home/<>/git/slam-ui/node_modules/@betterer/betterer/src/betterer.ts:31:10)
      at async Command.<anonymous> (/home/<>/git/slam-ui/node_modules/@betterer/cli/src/start.ts:38:28)
      at async Command.parseAsync
  (/home/<>/git/slam-ui/node_modules/@betterer/cli/node_modules/commander/lib/command.js:923:5)
      at async Object.cli__ (/home/<>/git/slam-ui/node_modules/@betterer/cli/src/cli.ts:58:3)

Error:Must use import to load ES Module: /home/<>/git/slam-ui/node_modules/@angular/compiler-cli/bundles/index.js
      require() of ES modules is not supported.
      require() of /home/<>/git/slam-ui/node_modules/@angular/compiler-cli/bundles/index.js from
      /home/<>/git/slam-ui/node_modules/@betterer/angular/dist/angular.js is an ES module file as it is a .js file
       whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope
      as ES modules.
      Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module"
      from /home/<>/git/slam-ui/node_modules/@angular/compiler-cli/package.json.

  require() of ES modules is not supported.
  require() of /home/<>/git/slam-ui/node_modules/@angular/compiler-cli/bundles/index.js from
  /home/<>/git/slam-ui/node_modules/@betterer/angular/dist/angular.js is an ES module file as it is a .js file
  whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES
  modules.
  Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from
  /home/<>/git/slam-ui/node_modules/@angular/compiler-cli/package.json.

      at Module._extensions..js (internal/modules/cjs/loader.js:1080:13)
      at Object.require.extensions.<computed> [as .js]
  (/home/<>/git/slam-ui/node_modules/@betterer/betterer/node_modules/ts-node/src/index.ts:1587:43)
      at Module.load (internal/modules/cjs/loader.js:928:32)
      at Function.Module._load (internal/modules/cjs/loader.js:769:14)
      at Module.require (internal/modules/cjs/loader.js:952:19)
      at require (internal/modules/cjs/helpers.js:88:18)
npm ERR! code ELIFECYCLE
phenomnomnominal commented 2 years ago

Hey! Sorry, I am still working away at this, but it's quite a lot of annoying refactoring of test frameworks and stuff. I haven't had a lot of time to look at it yet. Hoping to have something ready for ng-conf, so hopefully before the end of the month.

michealharrington commented 2 years ago

Great! Thank you for your effort. We’ve disabled betterer until it has been fixed so we are able to upgrade to angular 13.

On Aug 3, 2022, at 7:48 PM, Craig Spence @.***> wrote:

 Hey! Sorry, I am still working away at this, but it's quite a lot of annoying refactoring of test frameworks and stuff. I haven't had a lot of time to look at it yet. Hoping to have something ready for ng-conf, so hopefully before the end of the month.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

jpvanhal commented 1 year ago

I was able to find a workaround for this. You'll need to:

Here's a full example:

// .betterer.js
const { BettererFileTest } = require('@betterer/betterer');
const { BettererError } = require('@betterer/errors');
const { flattenDiagnosticMessageText } = require('typescript');

function angular(configFilePath, extraCompilerOptions) {
  if (!configFilePath) {
    throw new BettererError(
      "For `@betterer/angular` to work, you need to provide the path to a tsconfig.json file, e.g. `'./tsconfig.json'`. ❌",
    );
  }
  if (!extraCompilerOptions) {
    throw new BettererError(
      'For `@betterer/angular` to work, you need to provide compiler options, e.g. `{ strictTemplates: true }`. ❌',
    );
  }

  // Always has to do the full compile since a .component.html file needs to know about the module it lives in:
  return new BettererFileTest(async (_, fileTestResult, resolver) => {
    const absoluteConfigFilePath = resolver.resolve(configFilePath);

    const { performCompilation, readConfiguration } = await import(
      '@angular/compiler-cli'
    );

    const { rootNames, options } = readConfiguration(
      absoluteConfigFilePath,
      extraCompilerOptions,
    );

    const { diagnostics } = performCompilation({
      rootNames,
      options,
    });

    diagnostics.forEach((diagnostic) => {
      const { file, start, length } = diagnostic;
      const { fileName } = file;
      const result = fileTestResult.addFile(fileName, file.getFullText());
      const message = flattenDiagnosticMessageText(
        diagnostic.messageText,
        '\n',
      );
      result.addIssue(start, start + length, message);
    });
  });
}

module.exports = {
  'stricter template compilation': () =>
    angular('./tsconfig.json', {
      strictTemplates: true,
    }).include('./src/*.ts', './src/*.html'),
};
fdw commented 1 year ago

I don't want to add pressure, but I'm curious if the rewrite to ESM is already happening. Do you have news for us?

phenomnomnominal commented 1 year ago

Yeah I'm sorry, it turns out that I'm more coupled to cjs than expected and it's been really slow breaking that dependency, and I don't have a lot of time for this right now. Can't offer any timeframes but I am working on it when I get the time!

jacobbrokaw commented 1 year ago

I'm hoping to use this for an Angular 14 project. @phenomnomnominal, if it makes sense for you, let me know if there are any specific pieces you want someone to take a stab at. I'm no guru, but I'd be down to pitch in if the opportunity is there!

beaussan commented 11 months ago

@phenomnomnominal any way I can be of help here ? I'm trying to use it in a non angular setup, but still esm one. I love the tool and if there is any way I can help with the ESM migration, let me know!

phenomnomnominal commented 11 months ago

I have a branch where I’m trying to migrate the tests to Vite, which is the last big thing - it has some issues with hanging processes not exiting correctly. If anyone feels like checking that out it would be greatly appreciated!

jackw commented 10 months ago

any way I can be of help here ? I'm trying to use it in a non angular setup, but still esm one.

@beaussan I've just run into this issue whilst trying to migrate an application to use ViteJS with type: module in root package.json. After much head scratching I managed to get betterer to run by doing the following:

No idea why we need to pass --tsconfig. At a guess betterer internals are only checking if the config is a .js file else running it through typescript.