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

Testing after AppUI dropped CJS #1208

Open
aruniverse opened this issue Feb 6, 2025 · 9 comments
Open

Testing after AppUI dropped CJS #1208

aruniverse opened this issue Feb 6, 2025 · 9 comments
Assignees

Comments

@aruniverse
Copy link
Member

aruniverse commented Feb 6, 2025

Describe the bug

Posting on behalf of several internal teams who are now running into issues with their tests after AppUI dropped support for CJS. Would it be possible to publish some guides or provide some recommendations/examples on how teams should now migrate their testing when updating to AppUI 5.0?

@mathieu-fournier :

To resolve dependencies, Vitest uses NodeJs.
When resolving the packages, it will prioritize cjs over esm, when available.
Ex:

The issue arise when a package depending on AppUI has cjs. Ex :
Foo : cjs + esm
Appui: esm

Vitests, takes Foo : cjs and tries to require(Appui), this only work with cjs.
Since Appui does not have cjs, we are unable to build the tests.

Vitest dependency resolution

@a-gagnon :

FWIW I had some cases where vitest would choke on .scss that are transitively imported by our code, I had to "inline" the deps using the following:
Relevant read here: vitest-dev/vitest#2609

Maybe we should strive to drop sass variables/files from being exported in our packages at some point too!

@aruniverse aruniverse added bug Something isn't working and removed bug Something isn't working labels Feb 6, 2025
@GerardasB
Copy link
Collaborator

GerardasB commented Feb 6, 2025

Do we have a link to a minimal reproduction example that is causing the issue?
Here's a minimal working AppUI Vitest configuration.

Sub-dependencies that depend on AppUI packages should define a type field "type": "module" in their package.json (or use another method to indicate, that the ES modules should be used) https://nodejs.org/api/packages.html#determining-module-system

@a-gagnon
Copy link
Contributor

a-gagnon commented Feb 6, 2025

@GerardasB The problem is that packages that depend on appui weren't properly configured to fully and properly support ESM just yet. For civil, we had configs to output both CJS and ESM flavors, but we always used moduleResolution: node which is old and no longer works when consuming an ESM-only package. That module stuff is very confusing... Most likely we were consuming CJS to output our ESM? Anyhow, none of us knew we'd end up having issues.

Contrary to popular belief, we don't get to try out the latest RCs as soon as they come out to ensure everything works. That is time consuming and we do not have sufficient resources to have a developer do that full time. We usually ask consuming applications what date they expect to pull the latest major of (itwinjs/appui/etc), and we release our own packages a few weeks before that to ensure a smooth transition for them.

With studio requiring appui5, it puts everyone in a time crunch to update their packages and make them ESM compliant.

@mathieu-fournier
Copy link
Contributor

mathieu-fournier commented Feb 6, 2025

I did 2 minimal repro.

With packages that depends on appui.

First error. require() of esm module

Run :
npm install --force
npm test
It will take some time, but it'll fail.
https://stackblitz.com/edit/stackblitz-starters-woeqzw7j?file=package.json
Image


Second error. SCSS

Run :
npm install
npm test
It will take some time, but it'll fail.
https://stackblitz.com/edit/stackblitz-starters-sfjdkwco?file=vitest.config.ts
Image

@a-gagnon
Copy link
Contributor

a-gagnon commented Feb 6, 2025

Thanks @mathieu-fournier !
Yeah, the SCSS might not be directly caused by the CJS->ESM change, but it certainly outlines the problem.

This is yet another topic I don't have extensive knowledge of. Is there something we achieve with SASS that we couldn't do with raw CSS?

@mathieu-fournier
Copy link
Contributor

Thanks @mathieu-fournier ! Yeah, the SCSS might not be directly caused by the CJS->ESM change, but it certainly outlines the problem.

This is yet another topic I don't have extensive knowledge of. Is there something we achieve with SASS that we couldn't do with raw CSS?

True that, I update my comment with 2 minimal repo. thanks

@GerardasB
Copy link
Collaborator

GerardasB commented Feb 7, 2025

Thanks @mathieu-fournier ! Yeah, the SCSS might not be directly caused by the CJS->ESM change, but it certainly outlines the problem.

This is yet another topic I don't have extensive knowledge of. Is there something we achieve with SASS that we couldn't do with raw CSS?

I think importing the .css files rather than .scss would result in the same error and ideally we'd need to get rid of css/scss imports all together from packages (non-application code).

@GerardasB
Copy link
Collaborator

GerardasB commented Feb 7, 2025

First error. require() of esm module

Specifying an alias seems to be working as a workaround until the package is updated https://stackblitz.com/edit/stackblitz-starters-rcsyfvwu?file=vitest.config.ts

Second error. SCSS

This is analogous error due to additional packages importing .scss and .css files that is already solved with server.deps.inline. Simply add @itwin/presentation-hierarchies-react and @itwin/presentation-components to the inline dependencies array.

@GerardasB GerardasB self-assigned this Feb 7, 2025
@mathieu-fournier
Copy link
Contributor

First error. require() of esm module

Specifying an alias seems to be working as a workaround until the package is updated https://stackblitz.com/edit/stackblitz-starters-rcsyfvwu?file=vitest.config.ts

Second error. SCSS

This is analogous error due to additional packages importing .scss and .css files that is already solved with server.deps.inline. Simply add @itwin/presentation-hierarchies-react and @itwin/presentation-components to the inline dependencies array.

The examples where minimal, with only one dependency.
In our monorepo we have a lot of them, I'll try to set all the aliases and inlines. And give an update afterwards.
Thanks.

@mathieu-fournier
Copy link
Contributor

Update :
I added server.deps.inline and alias to the full monrepo.
And it worked !

The only other error I got was from mocking @itwin/core-frontend. This error on stackoverflow.
I was able to fix it by doing so :
Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants