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

feat: upgrade to Angular 17 #632

Closed
wants to merge 1 commit into from
Closed

Conversation

Tommy228
Copy link

  • ran ng update @angular/cli @angular/core
  • updated eslint dependencies
  • updated jest dependencies

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #631

What is the new behavior?

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Other information

Angular recently introduced TestBed.flushEffects() which is a helper function to execute pending effects . I think it makes sense to add spectator.flushEffects() but the method is still under dev preview, so maybe not the right time?

Copy link

stackblitz bot commented Nov 19, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Contributor

@chimurai chimurai left a comment

Choose a reason for hiding this comment

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

peerDependencies needs to be updated:

"@angular/common": ">= 16.0.0",
"@angular/router": ">= 16.0.0",
"@angular/animations": ">= 16.0.0"

@NetanelBasal
Copy link
Member

You can add spectator.flushEffects() and also introduce a an API to test defer views.

@chimurai
Copy link
Contributor

chimurai commented Nov 25, 2023

Running yarn build fails:

$ yarn build
yarn run v1.22.21
$ ng build --configuration production && yarn build:schematics && yarn copy:schematics && yarn copy:docs
Building Angular Package

------------------------------------------------------------------------------
Building entry point '@ngneat/spectator/internals'
------------------------------------------------------------------------------
✖ Compiling with Angular sources in Ivy partial compilation mode.
error TS2688: Cannot find type definition file for 'jasmine'.
  The file is in the program because:
    Entry point of type library 'jasmine' specified in compilerOptions
error TS2688: Cannot find type definition file for 'node'.
  The file is in the program because:
    Entry point of type library 'node' specified in compilerOptions

error Command failed with exit code 1.

Updating jest and @types/jest to latest and patching spectator/tsconfig.lib.json seems to fix it:

package.json:

  • "@types/jest": "29.5.10",
  • "jest": "29.7.0",
index adeaf7c4..8566c2b4 100644
--- a/projects/spectator/tsconfig.lib.json
+++ b/projects/spectator/tsconfig.lib.json
@@ -6,10 +6,10 @@
     "declaration": true,
     "inlineSources": true,
     "typeRoots": [
+      "../../node_modules/@types",
       "./typings"
     ],
     "types": [
-      "jasmine",
       "jest",
       "node"
     ],

-- edit 2023-12-02 --

Suggested incorrect patch.
Adding "../../node_modules/@types" at the end of typeRoots fixes the issue

https://devblogs.microsoft.com/typescript/announcing-typescript-5-1/

index adeaf7c4..8566c2b4 100644
--- a/projects/spectator/tsconfig.lib.json
+++ b/projects/spectator/tsconfig.lib.json
@@ -6,10 +6,10 @@
     "declaration": true,
     "inlineSources": true,
     "typeRoots": [
-       "./typings"
+       "./typings",
+      "../../node_modules/@types"
     ],

@Tommy228
Copy link
Author

Thanks, it's fixed now. I've tried running this on a big project under ng17 with yarn link but I'm having some typing errors

Property 'service' does not exist on type 'SpectatorService<TestService>'.

The generated dist looks fine though. I'm trying to understand why, maybe it's just an issue with my local setup.

@NetanelBasal
Copy link
Member

@Tommy228 try with verdaccio

@Tommy228
Copy link
Author

Thanks, that worked. I didn't know about it, pretty handy :)

I have just tried the upgrade on an Angular 17 project with 4000+ tests with Spectator and Jest, works fine.

@Tommy228 Tommy228 force-pushed the master branch 2 times, most recently from f963e6a to 777c67e Compare November 28, 2023 20:49
- ran ng update @angular/cli @angular/core
- updated eslint dependencies
- updated jest dependencies
@@ -6,10 +6,10 @@
"declaration": true,
"inlineSources": true,
"typeRoots": [
"../../node_modules/@types",
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this change, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

typescript got upgraded from 5.0.4 to 5.2.2

Typescript 5.1 introduced a change in typeRoots behaviour:

Explicit typeRoots Disables Upward Walks for node_modules/@types

More info: https://devblogs.microsoft.com/typescript/announcing-typescript-5-1/

Copy link
Contributor

@chimurai chimurai Dec 2, 2023

Choose a reason for hiding this comment

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

Looking at the Microsoft documentation the order of typeRoots should be corrected to:

    "typeRoots": [
      "./typings",
      "../../node_modules/@types"
    ],

"./typings"
],
"types": [
"jasmine",
Copy link
Member

Choose a reason for hiding this comment

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

Why we remove jasmine from here?

@@ -6,10 +6,10 @@
"declaration": true,
"inlineSources": true,
"typeRoots": [
"../../node_modules/@types",
Copy link
Contributor

@chimurai chimurai Dec 2, 2023

Choose a reason for hiding this comment

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

Looking at the Microsoft documentation the order of typeRoots should be corrected to:

    "typeRoots": [
      "./typings",
      "../../node_modules/@types"
    ],

"./typings"
],
"types": [
"jasmine",
Copy link
Contributor

Choose a reason for hiding this comment

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

jasmine type should remain.

@NetanelBasal
Copy link
Member

@Tommy228 your project is using jest or jasmine?

@Tommy228
Copy link
Author

Tommy228 commented Dec 2, 2023

My project uses jest, so I didn’t test it under jasmine. Indeed, we should add it back. I can’t update the PR before a few days so feel free to do it if you want to.

@lneumeier
Copy link
Contributor

My project uses jest, so I didn’t test it under jasmine. Indeed, we should add it back. I can’t update the PR before a few days so feel free to do it if you want to.

I created a new pull-request.
#633

@Tommy228 Tommy228 closed this Dec 17, 2023
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.

None yet

4 participants