search

kamilkisiela / apollo-angular

A fully-featured, production ready caching GraphQL client for Angular and every GraphQL server 🎁
https://apollo-angular.com
MIT License
1.5k stars 309 forks source link

Support angular 17 and drop 14,15,16 #2103

Closed rezoled closed 10 months ago

rezoled commented 10 months ago

Checklist:

changeset-bot[bot] commented 10 months ago

⚠️ No Changeset found

Latest commit: 80e296288910f81ebe11225c201ec75e555348de

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

PowerKiKi commented 10 months ago

my guess is that your schematics are trying to inject module-things into a standalone app, thus producing non-compiling code. #2093 addresses this by (trying to) supporting both module or standalone app. But I stumble on further issues. I'd suggest you to make a PR against my PR if you are able to find a solution.

rezoled commented 10 months ago

Do we really want to be sure that ng add works correctly? isn't this Angular's job? why do we need to test if an angular cli command works correctly? this is the problem of the test, it checks for something that ng-cli abstracts away

PowerKiKi commented 10 months ago

isn't this Angular's job?

Precisely, it is not Angular job. We have a non-trivial code in packages/apollo-angular/schematics that are meant to handle this. It is non-trivial because Angular does not provide a high-enough abstraction. Instead we need to juggle with quite a few things, and that makes it more fragile than it should be IMHO.

rezoled commented 10 months ago

Ah I see, I don't have a lot of experience messing around with schematics. It seems like running the integration test is only viable in the CI? is there a proper method to run the tests locally?

PowerKiKi commented 10 months ago

I'm not expert either. I've been reproducing https://github.com/kamilkisiela/apollo-angular/blob/1fd2b6f9e4eaafcdb0acf577a072755099fbdb80/.github/workflows/main.yml#L116-L125 manually locally. It's a long feedback loop, but I didn't find a better way yet ...

rezoled commented 10 months ago

I'm not expert either. I've been reproducing

https://github.com/kamilkisiela/apollo-angular/blob/1fd2b6f9e4eaafcdb0acf577a072755099fbdb80/.github/workflows/main.yml#L116-L125

manually locally. It's a long feedback loop, but I didn't find a better way yet ...

Thanks, that helps. I ran the test locally and found the source of the problem: 1) Angular's new generated app is standalone by default, so the prepare-e2e file which added an injection of apollo client did not add an import to the component of apollo module 2) cypress changed their default test, so prepare-e2e did no manage to modify the correct test and it failed because it was looking for content that did not exist.

I fixed prepare-e2e file for these scenarios and added a script to run the e2e locally.

rezoled commented 10 months ago

@kamilkisiela can we merge this? it's currently stopping devs from upgrading to Angular 17

PowerKiKi commented 10 months ago

Can you rebase this on top of my pr, branch powerkiki, so I have less code code to review when I get to review this, please?

rezoled commented 10 months ago

Can you rebase this on top of my pr, branch powerkiki, so I have less code code to review when I get to review this, please?

There you go: https://github.com/kamilkisiela/apollo-angular/pull/2104

PowerKiKi commented 10 months ago

Superseded by #2093