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 to Angular 14 #314

Closed
wants to merge 6 commits into from
Closed

Update to Angular 14 #314

wants to merge 6 commits into from

Conversation

pimmerks
Copy link

@pimmerks pimmerks commented Jun 3, 2022

Description

Updated packages to Angular v14.

Checklist

This PR works, but is not fully done, here is some stuff that needs to happen:

  • Testing locally seems to be working, but need to make sure it works for others
  • Linting has been removed because TSLint was deprecated, so ESLint should be added back.
  • Check if this library still works for older angular releases after this update (>=9 <=14)
  • It seems that the @briebug/cypress-schematic is deprecated so migrate to @cypress/schematic
  • RXJS should be updated v6 > v7
  • Newest version of typedoc is not working (out of the box, running npm run docs)

Fixes #313

@pimmerks pimmerks requested a review from a team as a code owner June 3, 2022 17:28
@pimmerks pimmerks marked this pull request as draft June 3, 2022 17:30
"@types/jasminewd2": "~2.0.3",
"@types/node": "^12.19.8",
"@types/node": "^12.11.1",
Copy link

@palexcast palexcast Jun 3, 2022

Choose a reason for hiding this comment

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

From Angulars changelog for NG 14.

Support for Node.js v12 has been removed as it will become EOL on 2022-04-30. Please use Node.js v14.15 or later.

Not sure if that means you should update @types/node to a later version > 14.15?

Copy link
Author

Choose a reason for hiding this comment

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

When creating a new angular 14 project, this dependency does not even get added, I think I will remove it.

Copy link
Member

@frederikprijck frederikprijck Jun 5, 2022

Choose a reason for hiding this comment

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

When creating a new angular 14 project, this dependency does not even get added

We have multiple dependencies that aren't added when you create a new Angular 14 project.
Let's refrain from removing dependencies if you are not aware of any reason they should or shouldn't be removed.

@pimmerks
Copy link
Author

pimmerks commented Jun 3, 2022

Some more stuff I have found:

An 'easy' way to fix might be to generate a new angular 14 project, copy over source code, selectively install new packages and use/configure new Cypress, ESLint, etc

@frederikprijck
Copy link
Member

frederikprijck commented Jun 5, 2022

Thanks for the PR, I know this is a draft PR but let me reply to a couple of things.

Please be aware that creating issues (and discussing) before opening PRs can be helpful to avoid unnecessary work. I can see there are more changes in this PR than should be required to work with Angular 14.

Even more so, as far as I am aware, our SDK should work with Angular 14 as-is. But we do need to bump the peerDependencies to ensure we can install the SDK without any warnings or need to provide a flag to npm install.

An 'easy' way to fix might be to generate a new angular 14 project, copy over source code, selectively install new packages and use/configure new Cypress, ESLint, etc

I don't think we need to do that. However, we are happy to bring our dependencies up to date. But this PR seems to be updating some, and downgrading others. This would need a bigger elaboration on what's happening in this PR. See #243 for an example PR.

TSLint -> ESLint migration

As much as I believe going with ESLint is the way to do, I don't see this as a strict need in order to support Angular 14 (the linter we use shouldn't affect our consumers). Not sure why you got rid of the linter to begin with.

@briebug/cypress-schematic is deprecated so we should migrate to @cypress/schematic

Same as above, I agree we should update, but that shouldn't block using this SDK with Angular 14.

RXJS should be updated v6 > v7

Same as above, RXJS is not a peerDepenedency or dependency from our SDK. So the version of rxjs used shouldn't have any impact on our users.

Happy to review a PR if you would provide one that updates with the minimum needed to support Angular 14 (so not moving our repository to Angular 14). Happy to review a follow up PR that bumps anything else (including Angular to 14, rxjs, ...), but let's keep the PR's small enough so we can ship it sooner rather than later.

I would also suggest to not update dependencies that don't seem to work and have nothing to do with updating to Angular 14 (e.g. typedoc).

@frederikprijck
Copy link
Member

FYI, I added a PR to fix #313 , see #315

@pimmerks
Copy link
Author

pimmerks commented Jun 6, 2022

I agree @frederikprijck, this repo needs some love but I don't have time for it right now unfortunately. Bumping the peerDependency is a quick and easy fix and I am happy as long as it works with Angular 14 (but it still feels a bit dirty though).
I will close this and retry again if I have some time on my hand. Thanks for your input and PR!

@pimmerks pimmerks closed this Jun 6, 2022
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.

Angular v14 not supported
3 participants