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

bug: upgrade to v2.14.0 breaks jest tests using esm #3251

Closed
3 tasks done
johncrim opened this issue Feb 23, 2022 · 6 comments · Fixed by #4136
Closed
3 tasks done

bug: upgrade to v2.14.0 breaks jest tests using esm #3251

johncrim opened this issue Feb 23, 2022 · 6 comments · Fixed by #4136
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@johncrim
Copy link

Prerequisites

Stencil Version

2.14.0

Current Behavior

Tests using external jest.config.js file (necessary to transform .mjs and .js files using ES modules) work fine using @stencil/core 2.13.0 . Upgrading to 2.14.0 results in errors like:

    C:\src\github\johncrim\repro-stencil-jest\libs\util\util.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import { Observable, NEVER } from 'rxjs';
                                                                                      ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      1 | import { Component, Prop, h, Element } from '@stencil/core';
      2 | import { format } from '../../utils/utils';
    > 3 | import { resizeObservable } from '../../../libs/util/util';
        | ^
      4 | import { Subscription } from 'rxjs';
      5 |
      6 | @Component({

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1728:14)
      at Object.<anonymous> (src/components/my-component/my-component.tsx:3:1)

This occurs in tests with dependencies on external ESM files, which have dependencies on other external ESM files.

The external jest config adds support for processing .mjs files, and that's about it:

# jest.config.js
module.exports = {
  preset: '@stencil/core/testing',
  testRegex: '/src/.*\\.spec\\.(ts|tsx|js)$',
  testRunner: 'jest-jasmine2',
  transform: {
    '^.+\\.(ts|tsx|jsx|css|mjs)$': '@stencil/core/testing/jest-preprocessor.js',
  }
};

Expected Behavior

No breaking change in test running when upgrading to @stencil.core 2.14.0

Steps to Reproduce

git clone git@github.com:johncrim/repro-stencil-jest.git
npm install
npm run jest

Yields this error:

    C:\src\github\johncrim\repro-stencil-jest\libs\util\util.mjs:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import { Observable, NEVER } from 'rxjs';
                                                                                      ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      1 | import { Component, Prop, h, Element } from '@stencil/core';
      2 | import { format } from '../../utils/utils';
    > 3 | import { resizeObservable } from '../../../libs/util/util';
        | ^
      4 | import { Subscription } from 'rxjs';
      5 |
      6 | @Component({

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1728:14)
      at Object.<anonymous> (src/components/my-component/my-component.tsx:3:1)

To fix, downgrade to @stencil/core version 2.13.0 :

npm add @stencil/core@2.13.0
# Clear jest cache
npm run jest -- --clear-cache
npm run jest

Code Reproduction URL

https://github.com/johncrim/repro-stencil-jest

Additional Information

I found a number of other bug reports in this repo reporting the same error message; however this is a regression from 2.13.0 to 2.14.0, so I felt it warranted a separate bug.

This is topically related to another feature request, which I have started working on: #3230 . However for this bug, everything is running in traditional jest mode (CJS), which means all ES modules have to be transpiled to CJS.

@ionitron-bot ionitron-bot bot added the triage label Feb 23, 2022
@rwaskiewicz
Copy link
Member

Thanks! I'll label this to get ingested into our backlog

@rwaskiewicz rwaskiewicz changed the title regression: Upgrade to 2.14.0 breaks jest tests: Cannot use import statement outside a module bug: upgrade to v2.14.0 breaks jest tests using esm Feb 23, 2022
@rwaskiewicz rwaskiewicz added Bug: Validated This PR or Issue is verified to be a bug within Stencil Feature: Testing and removed triage labels Feb 23, 2022
@johncrim johncrim mentioned this issue Feb 24, 2022
14 tasks
@alicewriteswrongs
Copy link
Contributor

alicewriteswrongs commented Mar 8, 2023

Hey @johncrim, I just opened a PR (#4136) which I believe will fix this issue. I've created a dev build with the change which can be installed as follows:

npm i @stencil/core@3.1.0-dev.1678312405.eddfff3

If you're able to confirm that it works for you as well that would be a great help!

@johncrim
Copy link
Author

johncrim commented Mar 8, 2023

I @alicewriteswrongs - I also created a PR that fixes the issue:

#3256

I have not tested your fix yet (or looked at the differences between your PR and mine), but will do so when I have the time.

@alicewriteswrongs
Copy link
Contributor

Ah apologies that I missed that you had already submitted a PR. I haven't taken a thorough look yet, but I'll say first off that the PR I just put up it quite a bit more minimal - it doesn't attempt to leverage Jest's --experimental-vm-modules, instead it just basically adds .mjs files to the set of files which will get transpiled to commonjs in the existing Jest setup (it turns out we only need to change 5 lines for that).

After reading through all of the related code I do think that the intent when the Jest preprocessor and whatnot was written was for .mjs files to be supported, so I think of the change in #4136 as a bug fix, whereas I think supporting --experimental-vm-modules is more of a new feature. Doesn't mean we can't do both! But one thing the Stencil team is working on now is an overhaul / assessment of our Jest setup, so there may be opportunities in the near future to make changes like that.

alicewriteswrongs added a commit that referenced this issue Mar 13, 2023
This adds support for importing from ES modules (w/ the `.mjs`
extension) in spec tests by ensuring that

1. files with such an extension are passed to the jest preprocessor
   (which then runs them through typescript)
2. our typescript helper for string-to-strong transpilation will emit
   `.mjs` files instead of just dropping them on the floor

Together these changes will allow running specs which import `.mjs`
files.

See #3251 for the original issue report
@johncrim
Copy link
Author

Thanks for the info @alicewriteswrongs .

Browsers support ESM, and jest/node supports ESM; though when I wrote this bug and wrote the fix the node support required the --experimental-vm-modules flag; and ionic's rationale for not prioritizing this seems to be that it was still experimental. Now node support for ESM it no longer experimental.

Whether it's a feature or a bug, I just want to be able to use ESM in tests without transpilation to CJS - all our other jest tests and the rest of our codebase have used ESM for over a year.

@alicewriteswrongs
Copy link
Contributor

Totally understand where you're coming from. We want to get ESM support for Stencil tests stood up too. As I mentioned above, we're starting to do some larger scale planning and work around Stencil's test infrastructure, and while I can't share a definite timeline right now we'll definitely be looking at ESM support as part of that. We'll also keep #3230 open to track the feature request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants