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

Jest ESM: Avoiding UMD imports #751

Closed
johncrim opened this issue Jan 21, 2021 · 14 comments · Fixed by #892
Closed

Jest ESM: Avoiding UMD imports #751

johncrim opened this issue Jan 21, 2021 · 14 comments · Fixed by #892
Labels
🚀 Feature Request new suggested feature

Comments

@johncrim
Copy link
Contributor

johncrim commented Jan 21, 2021

By default, Jest loads imports from UMD files. But with new ESM support in node and Jest, that should no longer be the case when using ESM - both external dependencies and internal dependencies should use ESM files, whenever available. This is particularly important since UMD is ES5 only, which can result in errors when running tests that are not errors in the lib or app.

More precisely: When using ESM, imports from angular package format should be loaded from ESM files, whether those angular packages are under node_modules or in a local directory.

After updating to use Jest 17 and [email protected], and the new jest-preset-angular/presets/defaults-esm, I was running into test perf issues (particularly on startup) that are significant relative to jest-preset-angular@8. I've been trying 2 approaches to improve test perf:

  • Reducing the number of files that have to be compiled to run tests, by using pre-compiled imports instead of source file references. Eg in jest.config changing from:
    moduleNameMapper: { '^@corp/([a-z\\-\/]+)': '<rootDir>/libs/$1/src/public-api.ts' }
    to:
    moduleNameMapper: { '^@corp/([a-z\\-\/]+)': '<rootDir>/dist/$1' }
    where dist is the output directory for project builds.
  • Using isolatedModules

Both of these are helping with perf, but I'm seeing issues with both too.

The problem I'm seeing with using pre-compiled imports is that UMD imports are used. While I have a workaround for this, I also think it's incorrect behavior, which is why I'm raising this issue.

Repro case

I've created a repro case for this. Prerequisites are node 15 and yarn.

git clone [email protected]:johncrim/repro-ivy-jest-preset-angular.git
git checkout bug/esm-jest

yarn
yarn build
yarn test

This repro case builds a library with 2 submodules, then runs a jest test (with ESM enabled) that uses a language feature that doesn't work in ES5/UMD, but that does work in ES2015.

Note that the identical karma test succeeds (b/c it doesn't use UMD):

yarn build # if not already run
yarn karma:single

Workaround

A workaround for this issue is to change the moduleNameMapper in jest.config.js to explicitly load non-UMD js files. Comment this line out:

  '^@this/(.*)$': '<rootDir>/dist/$1'

And uncomment these 2 lines:

  // '^@this/([a-z\\-]+)/([a-z\\-]+)$': '<rootDir>/dist/$1/fesm2015/this-$1-$2.js',
  // '^@this/([a-z\\-]+)': '<rootDir>/dist/$1/fesm2015/this-$1.js'

then:

yarn test

I don't know if this is an issue when importing files from angular libs in node_modules. It appears that when the config is <rootDir>/dist/$1 jest is reading the package.json file in the dist subdirectory, and then resolving that to the UMD js file.

🚀 Feature Proposal

I would like to see a way to make Jest smarter about using ESM or FESM files from angular package format references, so that all devs don't have to figure out this workaround. It also should be consistent whether the angular package is in a local directory, or under node_modules.

@johncrim johncrim added the 🚀 Feature Request new suggested feature label Jan 21, 2021
@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 21, 2021

I just found out about this today. When not using ESM, we can skip processing umd and when using ESM, we can skip processing ESM file.

@johncrim
Copy link
Contributor Author

I can remove the moduleNameMapper property in jest.config.js, and use moduleDirectories instead:

  moduleDirectories: [
    'node_modules',
    'dist'
  ],
...

(which requires building the lib into dist/@this/ui-lib). This seems like a "nicer" way to go, but it's still loading the UMD modules instead of ESM. This indicates to me that it's probably also loading UMDs for external libraries in node_modules.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 22, 2021

Oh actually my previous comment was about something else, partially related.

Regarding to loading ESM when running Jest with ESM, it is possible to do it via custom Jest resolver. One example is from nrwl nx https://github.com/nrwl/nx/blob/master/packages/jest/plugins/resolver.ts

Ideally, a custom resolver should contain the logic to load the correct format, UMD when running in normal mode and ESM when running in ESM mode based on Angular package format, anything else would just fallback to default Jest resolver.

@johncrim
Copy link
Contributor Author

Thanks for the pointer. Do you think this is something that jest-preset-angular should handle, or should it be separate?

Stepping through a bunch of jest code today, it looks like import resolution (ala jest-resolve/defaultResolver) is separate from the decision to load the import as an ESM module. jest-resolve/shouldLoadAsEsm only returns true if there's a package.json file and it contains type === 'module'. That field isn't present in my submodule package.json files (I'll try adding it). Also, zone.js and other angular libs are being loaded as UMDs.

It turns out that my attempt to load FESM files instead of UMDs (via moduleNameMapper: '^@this/([a-z\\-]+)/([a-z\\-]+)$': '<rootDir>/dist/$1/fesm2015/this-$1-$2.js') is still resulting in them not being loaded as ESM modules. Apparently they're loaded as CommonJs modules, which works most of the time. I wonder if there's associated perf overhead though.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 22, 2021

Thanks for the pointer. Do you think this is something that jest-preset-angular should handle, or should it be separate?

I think it should be. If running in ESM mode, this repo at least should have a custom jest resolver or a specific moduleNameMapper config that is included in defaults-esm preset to make sure that Jest always passes ESM built files to the Jest transformer https://github.com/thymikee/jest-preset-angular/blob/master/src/ng-jest-transformer.ts

Stepping through a bunch of jest code today, it looks like import resolution (ala jest-resolve/defaultResolver) is separate from the decision to load the import as an ESM module. jest-resolve/shouldLoadAsEsm only returns true if there's a package.json file and it contains type === 'module'. That field isn't present in my submodule package.json files (I'll try adding it). Also, zone.js and other angular libs are being loaded as UMDs.

It turns out that my attempt to load FESM files instead of UMDs (via moduleNameMapper: '^@this/([a-z\\-]+)/([a-z\\-]+)$': '<rootDir>/dist/$1/fesm2015/this-$1-$2.js') is still resulting in them not being loaded as ESM modules. Apparently they're loaded as CommonJs modules, which works most of the time. I wonder if there's associated perf overhead though.

I don’t get “not loaded as ESM modules”, does it mean the Jest transformer doesn’t receive ESM files ?

Regarding to perf, in general the goal to improve is reducing the most amount of processing tasks for the Jest transformer.

According to recent experiments, when running in CommonJs mode, it is possible to skip the whole type checking as well as compilation task when dealing with umd files which are built by Angular. That is expected because UMD is compatible with CommonJs which can be a slight performance boost.

According to https://dev.to/iggredible/what-the-heck-are-cjs-amd-umd-and-esm-ikm, UMD is a fallback when ESM doesn’t work. This means the potential logic for the custom Jest resolver: if can’t find ESM files when running in ESM mode(1st try with default resolver, if not found then fallback to custom logic to find ESM), simply fallback to load UMD (default Jest resolver I assume)

@johncrim
Copy link
Contributor Author

johncrim commented Jan 22, 2021

Thank you for the info. I agree, I think it would be great if jest-preset-angular made this work nicely.

I did some more research/code stepping. Things you may be aware of but are worth pointing out in this issue:

As you said @ahnpnl, the resolver can be overridden, which could be done to resolve non UMD files from angular packages (both in node_modules and locally). The default resolver completely relies on the browserify/resolve library. The logic that uses the package.json "main" field to find the UMD file is here:
https://github.com/browserify/resolve/blob/master/lib/sync.js#L161

This default resolver is not ES2015 aware, so by default (unless a resolver is configured) Jest is always going to resolve UMD files in Angular packages (and most other packages). IMO this is a Jest bug that should be fixed as part of jestjs/jest#9430 , but I don't know if the Jest team will agree. It would be great if there was an equivalent library that provides similar module resolution logic to browserify/resolve, but is ES2015 aware - given the node support for ESM, I would think there would be a need for such a thing.

There is separate logic to determine whether the resolved module should be loaded as an ESM module or not. Here is the top level code:

    const [path, query] = specifier.split('?');

    const resolved = this._resolveModule(referencingIdentifier, path);

    if (
      this._resolver.isCoreModule(resolved) ||
      this.unstable_shouldLoadAsEsm(resolved)
    ) {
      return this.loadEsmModule(resolved, query, isStaticImport);
    }

    return this.loadCjsAsEsm(referencingIdentifier, resolved, context);

The unstable_shouldLoadAsEsm() method eventually calls cachedPkgCheck :

function cachedPkgCheck(cwd: Config.Path): boolean {
  const pkgPath = escalade(cwd, (_dir, names) => {
    if (names.includes('package.json')) {
      // will be resolved into absolute
      return 'package.json';
    }
    return false;
  });
  if (!pkgPath) {
    return false;
  }

  let hasModuleField = cachedChecks.get(pkgPath);
  if (hasModuleField != null) {
    return hasModuleField;
  }

  try {
    const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8'));
    hasModuleField = pkg.type === 'module';
  } catch {
    hasModuleField = false;
  }

  cachedChecks.set(pkgPath, hasModuleField);
  return hasModuleField;
}

As you can see, loadEsmModule() is only called if the package.json contains "type": "module". Even if we replace the resolver to return the ES2015 modules for angular imports, each of those modules will be loaded using loadCjsAsEsm(), since none of the angular (or zone,js, or ....) package.json files contain "type": "module".

I don't yet know what the difference in behavior is between loadEsmModule() and loadCjsAsEsm(). It does appear that loadCjsAsEsm() works even if passed an ESM module - I say this only because I've watched it work when the moduleNameMapper value '^@this/([a-z\\-]+)/([a-z\\-]+)$': '<rootDir>/dist/$1/fesm2015/this-$1-$2.js' is used to return an ESM module. But I don't know if it's optimal, and it does look like stuff is put into separate module registries depending on whether the file is determined to be CJS or ESM.

I think it's bad news that the resolver can be overridden, but the shouldLoadAsEsm() logic can't (that I'm aware of). They're clearly coupled, I think it would be preferable if the Jest implementation were designed so that the resolver determines whether the returned module is ESM or not.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 22, 2021

My guess for loadCjsAsEsm is a fallback for ESM since UMD works everywhere and usually used as a fallback in case ESM does not work.

Anyways from transformer side, the work will be only creating a custom resolver.

@johncrim
Copy link
Contributor Author

It looks like jestjs/jest#9771 may handle the module resolution for us, though it might take a while - jest is waiting on browserify to implement it.

RE the "is this module ESM?" logic, I made a comment here requesting that resolvers gain the ability to specify the module format of the returned file.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 22, 2021

nice, I think let's put this on hold for now while waiting for Jest team's opinion.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Mar 1, 2021

It seems like [email protected] module resolution somehow smarter that it doesn't pass UMD files to transformer anymore. Would you please check to see if you observe the same thing ?

@johncrim
Copy link
Contributor Author

johncrim commented Mar 12, 2021

No luck - [email protected] and [email protected] (with [email protected]) are still loading + running umd files both for angular libs and internal angular libs. I don't know how to check what is being passed to the transformer, but I'm dumping the call stack from the internal lib using console, and get eg:

  console.warn
    Creating DerivedComponent

      321 |             var _this = _super.call(this, elementRef) || this;
      322 |             _this.on = false;
    > 323 |             console.warn('Creating DerivedComponent');
          |                     ^
      324 |             return _this;
      325 |         }
      326 |         Object.defineProperty(DerivedComponent.prototype, "disabled", {

      at new DerivedComponent (dist/ui-lib/bundles/this-ui-lib-derived.umd.js:323:21)
      at NodeInjectorFactory.DerivedComponent_Factory [as factory] (dist/ui-lib/bundles/this-ui-lib-derived.umd.js:355:75)
      at getNodeInjectable (../packages/core/src/render3/di.ts:613:38)
      at instantiateRootComponent (../packages/core/src/render3/instructions/shared.ts:1135:7)
      at createRootComponent (../packages/core/src/render3/component.ts:225:21)
      at ComponentFactory.Object.<anonymous>.ComponentFactory.create (../packages/core/src/render3/component_ref.ts:215:19)
      at initComponent (../packages/core/testing/src/r3_test_bed.ts:355:28)

Same as before, even if I use a module mapper workaround to try to explicitly load the FESM module for internal libs:

  moduleNameMapper: {
    // Old form loads umd modules, eg:
    //'^@this/(.*)$': '<rootDir>/dist/$1'
    // Required to load fesm2015
    '^@this/([a-z\\-]+)/([a-z\\-]+)$': '<rootDir>/dist/$1/fesm2015/this-$1-$2.js',
    '^@this/([a-z\\-]+)': '<rootDir>/dist/$1/fesm2015/this-$1.js'
  },

When I do this, the internal FESM module loads, but it's still using UMD versions of angular libs, like zone-testing-bundle.umd.js:

    Trace: Setting disabled: true
        at DerivedComponent.set disabled (C:\src\cc\test-cases\repro-ivy-jest-preset-angular\dist\ui-lib\fesm2015\this-ui-lib-base.js:13:17)
        at DerivedComponent.set disabled [as disabled] (C:\src\cc\test-cases\repro-ivy-jest-preset-angular\dist\ui-lib\fesm2015\this-ui-lib-derived.js:23:27)
        at C:\src\cc\test-cases\repro-ivy-jest-preset-angular\test-cases\umd-libs\derived.component.spec.ts:26:39
        at ZoneDelegate.Object.<anonymous>.ZoneDelegate.invoke (C:\src\cc\test-cases\repro-ivy-jest-preset-angular\node_modules\zone.js\bundles\zone-testing-bundle.umd.js:407:30)
        at ProxyZoneSpec.Object.<anonymous>.ProxyZoneSpec.onInvoke (C:\src\cc\test-cases\repro-ivy-jest-preset-angular\node_modules\zone.js\bundles\zone-testing-bundle.umd.js:3765:43)
        at ZoneDelegate.Object.<anonymous>.ZoneDelegate.invoke (C:\src\cc\test-cases\repro-ivy-jest-preset-angular\node_modules\zone.js\bundles\zone-testing-bundle.umd.js:406:56)
        at Zone.Object.<anonymous>.Zone.run (C:\src\cc\test-cases\repro-ivy-jest-preset-angular\node_modules\zone.js\bundles\zone-testing-bundle.umd.js:167:47)
        at Object.wrappedFunc (C:\src\cc\test-cases\repro-ivy-jest-preset-angular\node_modules\zone.js\bundles\zone-testing-bundle.umd.js:4250:34)
        at Promise.then.completed (C:\src\cc\test-cases\repro-ivy-jest-preset-angular\node_modules\jest-circus\build\utils.js:307:28)
        at new Promise (<anonymous>)

I've committed my changes (upgrading dependencies), so you can use the directions above to check out my test repo and see the same.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Mar 19, 2021

Since Jest already takes care of loading the correct module from node_modules so for this issue, this preset will only take care of loading the correct zone-testing file.

Regarding to built files which are outside node_modules like Angular libraries, users have to configure moduleNameMapper or make a custom resolver by themselves if they want to load ESM files because different projects have different settings. This preset won’t take care of that but will add documentation about tips when working with ESM.

@johncrim
Copy link
Contributor Author

johncrim commented Mar 19, 2021

Will Jest handle correctly loading existing ESM files from angular package format packages within node_modules? I don't think it will (but haven't tested that in recent builds).

When I last reviewed the Jest/resolver code, it requires "type": "module" in the package.json to load the module as ESM, and none of the angular packages or key dependencies (rxjs, zone.js) have "type": "module" in their package.json. Neither do angular package format packages.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Mar 19, 2021

Yes Jest does resolving correctly ESM files from node_modules.

ngcc-jest-processor: running ngcc
(node:2374) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/setup-jest.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/jest-global-mocks.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/heroes/hero.service.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/heroes/hero.service.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/node_modules/tslib/tslib.es6.js
PASS src/__tests__/heroes/hero.service.spec.ts (5.626 s)
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/animations/animations.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/__helpers__/index.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/animations/animations.component.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/__helpers__/config.helpers.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/animations/animations.component.html
PASS src/__tests__/animations/animations.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/on-push/on-push.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/on-push/on-push.component.ts
PASS src/__tests__/on-push/on-push.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/heroes/heroes.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/heroes/heroes.component.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/heroes/heroes.component.html
PASS src/__tests__/heroes/heroes.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/jest-globals/jest-globals.test.ts
PASS src/__tests__/jest-globals/jest-globals.test.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/async/async.test.ts
PASS src/__tests__/async/async.test.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/ngc-compiled-lib/ngc-compiled-lib.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/ngc-compiled-lib/ngc-compiled-lib.component.ts
PASS src/__tests__/ngc-compiled-lib/ngc-compiled-lib.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/medium/medium.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/medium/child.component.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/medium/medium.component.ts
PASS src/__tests__/medium/medium.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/forward-ref/forward-ref.spec.ts
PASS src/__tests__/forward-ref/forward-ref.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/simple-with-styles/simple-with-styles.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/simple-with-styles/simple-with-styles.component.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/simple-with-styles/simple-with-styles.component.html
PASS src/__tests__/simple-with-styles/simple-with-styles.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/dynamic-component/dynamic-host.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/dynamic-component/dynamic-host.component.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/dynamic-component/dynamic-host.module.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/dynamic-component/dynamic-button.component.ts
PASS src/__tests__/dynamic-component/dynamic-host.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/calc/calc.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/calc/calc.component.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/__mocks__/image.js
PASS src/__tests__/calc/calc.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/ng-lib/my-lib.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/ng-lib/my-lib.component.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/ng-lib/disableable.directive.ts
PASS src/__tests__/ng-lib/my-lib.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/ng-reflect-as-text/ng-reflect-as-text.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/ng-reflect-as-text/ng-reflect-as-text.component.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/ng-reflect-as-text/ng-reflect-as-text.component.html
PASS src/__tests__/ng-reflect-as-text/ng-reflect-as-text.component.spec.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/done-callback/done-callback.test.ts
PASS src/__tests__/done-callback/done-callback.test.ts
filePath /home/runner/work/jest-preset-angular/jest-preset-angular/e2e/test-app-v10/src/__tests__/custom-typings/index.spec.ts
PASS src/__tests__/custom-typings/index.spec.ts

See https://github.com/thymikee/jest-preset-angular/runs/2151739998?check_suite_focus=true

Before [email protected], all umd files would be passed into ts-jest to transform in ESM mode., now it's no longer the case.

The same for umd files are no longer required to be transformed in CommonJS mode since Jest detects correctly that umd is compatible with commonjs

So now in either CommonJS or ESM mode, Angular packages are no longer required to be compiled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 Feature Request new suggested feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants