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

Update vite and vitest, compile and fix tests #410

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

kaisalmen
Copy link
Contributor

@kaisalmen kaisalmen commented Dec 6, 2023

vite is now explicitly included as devDependency, so its version is more transparent.

There were a couple of undetected compilation errors in spec files. Therefore special tsconfig.test.json with noEmit=true have been added. They are included in the main compilation now. tsconfig.src.json cover the regular code compilation. tsconfig.json are required for vscode/eslint only now (we do the same in other projects, e.g. Langium). Alle "type": [] usages has been removed from tsconfig files as they shadows type issues. Therefore skipLibCheck is needed in test configs to shadow snabbdom and rollup type definition errors.

Node version in GitHub Action is upgraded to node 18. vite v5 requires it.
Volta instructions have been added.
Test were unstable with new default setting. Reason is unknown. It could be inversify not being safe with regard to concurrency, but that is speculation. pool is now configured to use only one worker thread.

inversify and reflect-metadata have been updated to ~6.0.2 and ~0.1.14 respectively.

@kaisalmen kaisalmen changed the title Update vite and vitest and all other root devDependencies Update vite and vitest, compile and fix tests Dec 6, 2023
@dhuebner
Copy link
Contributor

dhuebner commented Dec 7, 2023

@kaisalmen
I still see @injectable() is marked with an error:

Unable to resolve signature of class decorator when called as an expression.
  The runtime will invoke the decorator with 2 arguments, but the decorator expects 1.

Is it an inversify <-> typescript version mismatch?

@kaisalmen
Copy link
Contributor Author

kaisalmen commented Dec 7, 2023

This should have been prevented by fixing the version to 6.0.1 (see description). It does not fail in GHA, that's why I think it is configured correctly. What does yarn list inversify say?

@dhuebner
Copy link
Contributor

dhuebner commented Dec 7, 2023

It says:

gitpod /workspace/sprotty (kaisalmen/vite-vitest-update) $ yarn list inversify
yarn list v1.22.21
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ [email protected]

@dhuebner
Copy link
Contributor

dhuebner commented Dec 7, 2023

@kaisalmen
I also noticed that the tasks.json is outdated. Maybe it can be fixed in this PR as well?
Just remove "* All" and "* Example" tasks.

@kaisalmen kaisalmen force-pushed the kaisalmen/vite-vitest-update branch 2 times, most recently from 15433c1 to fb1432d Compare December 7, 2023 16:11
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Dec 7, 2023

Just remove "* All" and "* Example" tasks.

Done.

vscode related problems should be fixed.

Tests execution is instable. The test themselves are bot broken. Code issues are fixed. Happens more often in the pipeline. Don't know why, yet. Will investigate

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running the tests in Gitpod I get

 FAIL  packages/sprotty/src/base/commands/command-stack.spec.ts [ packages/sprotty/src/base/commands/command-stack.spec.ts ]
Error: Expression expected
 ❯ error node_modules/rollup/dist/es/shared/parseAst.js:337:30
 ❯ nodeConverters node_modules/rollup/dist/es/shared/parseAst.js:2084:9
 ❯ convertNode node_modules/rollup/dist/es/shared/parseAst.js:969:12
 ❯ convertProgram node_modules/rollup/dist/es/shared/parseAst.js:960:48
 ❯ parseAstAsync node_modules/rollup/dist/es/shared/parseAst.js:2150:20
 ❯ ssrTransformScript node_modules/vite/dist/node/chunks/dep-YJaePtkC.js:49505:15
 ❯ loadAndTransform node_modules/vite/dist/node/chunks/dep-YJaePtkC.js:49093:11

examples/package.json Outdated Show resolved Hide resolved
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Dec 8, 2023

Tests execution is instable. The test themselves are bot broken. Code issues are fixed. Happens more often in the pipeline. Don't know why, yet. Will investigate

It may be vitest problem. I am not sure, yet. Happens very rarely on Windows, more likely on Linux.

@kaisalmen kaisalmen force-pushed the kaisalmen/vite-vitest-update branch 2 times, most recently from ed0eff3 to 3c9a3ef Compare December 8, 2023 09:02
@kaisalmen
Copy link
Contributor Author

@dhuebner and @spoenemann squashed all commits into one and removed all unrelated dependency changes. Dependency changes are only related to vite and vitest. vite v5 itself require node 18, so I kept that change.

All compiler adjustments and test fixes are kept as well.
I added volta instructions to all packages.json now, so everyone who uses it, now always has correct node and yarn versions.

The vitest instability is still not resolved. I am digging into this now...

- Fix vscode issue. Add proper tsc build configuration to examples integrate it into overall watch
- Fix references to local projects not resolved in vscode, fix lint, remove tasks
- Add test code compilation
- Only use one thread for test execution
- Update inversify and reflect-metadata
@kaisalmen
Copy link
Contributor Author

@dhuebner and @spoenemann this is now ready for review. Description is now up-to-date with performed changes.

Copy link
Contributor

@dhuebner dhuebner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No inversify type errors in the IDE, nice! :-) Tests are passing.
One thing we need to keep an eye on are regex inclusion exclusion patterns. We have a folder named lib in packages/sprotty/src/lib and the output folders are also named lib. However, right now it seams to work well!

packages/sprotty-elk/typings/elkjs/index.d.ts Outdated Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
@kaisalmen kaisalmen merged commit ee9698d into master Dec 9, 2023
2 checks passed
@spoenemann spoenemann deleted the kaisalmen/vite-vitest-update branch December 11, 2023 07:54
@spoenemann spoenemann added this to the v1.1.0 milestone Jan 3, 2024
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

Successfully merging this pull request may close these issues.

3 participants