+
Skip to content

fix(vite-plugin-angular): prevent context loss for "each" tests #1190

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 1, 2024
Merged

fix(vite-plugin-angular): prevent context loss for "each" tests #1190

merged 1 commit into from
Jul 1, 2024

Conversation

gultyayev
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • 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:

Which package are you modifying?

  • vite-plugin-angular
  • vite-plugin-nitro
  • vitest-angular
  • astro-angular
  • create-analog
  • router
  • platform
  • content
  • nx-plugin
  • trpc

What is the current behavior?

Currently, when zones are wrapping the test function the context is lost causing an error "Cannot read withContext() of undefined".

Closes #1115

What is the new behavior?

The original fn contains the mentioned withContext() method. I added reference to the initial method to the bounding function in order to preserve context.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I'm not sure how to properly test this change. I did not find tests for setup-vitest. I also failed to test this package using npm link as it failed complaining

failed to load config from /Users/crush/Projects/analog-vitest-repro/apps/analog-vitest/vite.config.mts
Error: Cannot find module '@angular/build/private'

The original fix was found by tinkering the setup-vitest.js file from node_modules from the repro in the issue. I applied the same changes to the .ts file.

[optional] What gif best describes this PR or how it makes you feel?

@netlify Netlify
Copy link

netlify bot commented Jul 1, 2024 •
edited
Loading

✅ Deploy Preview for analog-docs ready!

Name Link
🔨 Latest commit 05d796f
🔍 Latest deploy log https://app.netlify.com/sites/analog-docs/deploys/66831b858617ce000815c7bd
😎 Deploy Preview https://deploy-preview-1190--analog-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify Netlify
Copy link

netlify bot commented Jul 1, 2024 •
edited
Loading

✅ Deploy Preview for analog-blog ready!

Name Link
🔨 Latest commit 05d796f
🔍 Latest deploy log https://app.netlify.com/sites/analog-blog/deploys/66831b85b68d41000840accd
😎 Deploy Preview https://deploy-preview-1190--analog-blog.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify Netlify
Copy link

netlify bot commented Jul 1, 2024 •
edited
Loading

✅ Deploy Preview for analog-app ready!

Name Link
🔨 Latest commit 05d796f
🔍 Latest deploy log https://app.netlify.com/sites/analog-app/deploys/66831b851dcdfd0008cd2ea6
😎 Deploy Preview https://deploy-preview-1190--analog-app.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify Netlify
Copy link

netlify bot commented Jul 1, 2024 •
edited
Loading

✅ Deploy Preview for analog-ng-app ready!

Name Link
🔨 Latest commit 05d796f
🔍 Latest deploy log https://app.netlify.com/sites/analog-ng-app/deploys/66831b85884bd50008c4185e
😎 Deploy Preview https://deploy-preview-1190--analog-ng-app.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@brandonroberts
Copy link
Member

Thanks @gultyayev! Will you apply the change here also?

https://github.com/analogjs/analog/blob/beta/packages/vitest-angular/setup-zone.ts

@gultyayev
Copy link
Contributor Author

Thanks @gultyayev! Will you apply the change here also?

https://github.com/analogjs/analog/blob/beta/packages/vitest-angular/setup-zone.ts

Sure.

However, I just realised that somehow my fix works only when a test is run via WebStorm 🤔 If I run nx run analog-vitest:test it still gives the same error...

Now I feel even more related to the Gif

@brandonroberts
Copy link
Member

Thanks @gultyayev! Will you apply the change here also?
https://github.com/analogjs/analog/blob/beta/packages/vitest-angular/setup-zone.ts

Sure.

However, I just realised that somehow my fix works only when a test is run via WebStorm 🤔 If I run nx run analog-vitest:test it still gives the same error...

Now I feel even more related to the Gif

Oh, interesting 🤔

@gultyayev
Copy link
Contributor Author

gultyayev commented Jul 1, 2024 •
edited
Loading

Oh... I just realized why this is happening.

The two errors actually belong to different test suites. And don't originate from one simultaneously.

image

My fix fixes the global variables patching (test.service.spec.ts).
However, the second case, when we import helper functions directly from vitest is unhandled. I suppose, it's not handled initially by the plugin.

@brandonroberts
Copy link
Member

That's correct. This assumes you're using the patched globals

@gultyayev
Copy link
Contributor Author

That's correct. This assumes you're using the patched globals

Is there a way for us to also patch the imports?

@brandonroberts
Copy link
Member

That's correct. This assumes you're using the patched globals

Is there a way for us to also patch the imports?

Maybe, but I'd stay clear of doing that for now.

@gultyayev
Copy link
Contributor Author

gultyayev commented Jul 1, 2024 •
edited
Loading

I can see two points for improvements now:

  1. Update docs to mention this limitation
  2. Update the schematics & docs to also provide global types for TypeScript language service not to complain

I guess I could raise two separate MRs to followup.

@brandonroberts
Copy link
Member

I can see two points for improvements now:

  1. Update docs to mention this limitation
  2. Update the schematics & docs to also provide global types for TypeScript language service not to complain

I guess I could raise two separate MRs to followup.

That would be helpful, thanks

@brandonroberts brandonroberts merged commit 610f406 into analogjs:main Jul 1, 2024
24 checks passed
@brandonroberts
Copy link
Member

Thanks! @allcontributors add @gultyayev for code

@allcontributors allcontributors bot mentioned this pull request Jul 1, 2024
Merged
@allcontributors AllContributors
Copy link
Contributor

@brandonroberts

I've put up a pull request to add @gultyayev! 🎉

brandonroberts pushed a commit that referenced this pull request Jul 1, 2024
@gultyayev @brandonroberts
fix(vite-plugin-angular): prevent context loss for "each" tests (#1190)
13df344
@gultyayev gultyayev deleted the main branch July 1, 2024 21:34
@gultyayev gultyayev mentioned this pull request Jul 2, 2024
Merged
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonroberts brandonroberts brandonroberts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gultyayev @brandonroberts
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载