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(lib): upgrade to angular 10 #110

Merged
merged 5 commits into from
Oct 28, 2020
Merged

feat(lib): upgrade to angular 10 #110

merged 5 commits into from
Oct 28, 2020

Conversation

cyclist82
Copy link
Contributor

@cyclist82 cyclist82 commented Oct 23, 2020

This PR updates ngx-drag-to-select to work with an angular 10 application.

The main change is that the DragToSelectModule needed this migration.

The change to this module is backwards compatible with Angular 8.0.0.

Also updates dependencies of the test application to the latests versions. I skipped cypress related dependencies, because I think using the now implemented typescript support should be used when it's updated. But I think this should not be part of this PR.

@d3lm
Copy link
Owner

d3lm commented Oct 26, 2020

This looks pretty good. Thanks for the PR! I thought about removing RxJS as a peer-dependency but I think we should keep it in there. Modern package managers install the peer dependencies anyways, and it's no longer a user concern, at least npm 7 does that now and from what I read, it was a big mistake not to install them in the first place. Keeping RxJS as a peer-dep makes it more complete IMO. Curious to hear your thoughts.

If you agree, can you add it back in and just update the version? Then this PR is good to go and I ll go ahead and merge it.

@cyclist82
Copy link
Contributor Author

cyclist82 commented Oct 26, 2020

Thanks a lot for your remarks @d3lm. I'm absolutely fine with adding RxJS as a peer-dependency. I'm going to use a minimum version of RxJS 6.4.0, because that was used with angular 8.0.0. I'll add a commit soon.

When you'll do the next release with these changes are you going to mention the lost support for angular 6 and 7 in the release notes?

Copy link
Owner

@d3lm d3lm left a comment

Choose a reason for hiding this comment

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

Thanks! One last thing, which is we need to update the readme as well. Maybe we can add a section to the bottom about the support for Angular 6 and 7, and that this upgrade, as you mentioned, no longer supports Angular < 8. And we won't backport new features, only severe issues. So if people want the latest and greatest, they have to update to at least Angular 8.x.

There is already a section for "support for Angular 5.x", maybe we can add another one for "Angular 6 and 7".

To also mention this in the changelog, can you please update your commit message to contain the breaking change as well? As in:

feat(lib): upgrade to angular 10

BREAKING CHANGE: dropping support for Angular < 8.x

Many thanks!

Leif Lampater and others added 2 commits October 27, 2020 16:29
BREAKING CHANGE: dropping support for Angular < 8.x
@cyclist82
Copy link
Contributor Author

Thanks for the tips.

I added the requested changes to the commit message and updated the README. Please have a look if you like the changes @d3lm.

Copy link
Owner

@d3lm d3lm left a comment

Choose a reason for hiding this comment

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

We are getting there! Sorry for all the remarks. You're doing an amazing work here. Thanks so much for hanging in there and implementing all my feedback.

README.md Outdated
@@ -501,9 +501,16 @@ Example:

Yes.

### Does this library work with Angular 5.x?
### Does this library work with Angular Versions < 8.0.0?
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we go with a more general title, something along the lines of Can I use this library with an older version of Angular?

README.md Outdated

The latest version that supports Angular 5.x is 1.1.1. You can still install it via npm or yarn, e.g. `npm install ngx-drag-to-select@1.1.1`.
Please have a look here to see which version supports your angular application.
Copy link
Owner

Choose a reason for hiding this comment

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

I'd re-write this to something like (given we use the title I suggested):

Yep you totally can! But this also means you won't be able to use the latest and greatest and all of the newer features. The reason for this is that we are not back porting new features to older versions of this library, due to maintenance overhead. Severe bugs and security issues, however, are back ported.

What does that mean for you now? We recommend to stay up to date with new Angular versions. If for some reasons you can't then here's an overview versions you could use with older versions of Angular:

<TABLE>

To install a specific version run for example `npm install ngx-drag-to-select@x.x.x`.

And then we can also get rid of the sentence below the table.

README.md Outdated

You can still install it via npm or yarn, e.g. `npm install ngx-drag-to-select@x.x.x`.

| Angular Version | ngx-drag-to-select Version |
Copy link
Owner

Choose a reason for hiding this comment

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

Great idea! Thanks for this addition.

Copy link
Owner

Choose a reason for hiding this comment

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

Love it!!

Copy link
Owner

Choose a reason for hiding this comment

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

Let's change the headers to only Angular and ngx-drag-to-select. The "version" can be inferred by the user very quickly by looking at the rows.

README.md Outdated

| Angular Version | ngx-drag-to-select Version |
| -------------------- | -------------------------- |
| 5.x.x | 1.1.1 |
Copy link
Owner

Choose a reason for hiding this comment

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

I would use version ranges, e.g. <= 1.1.1.

README.md Outdated
| Angular Version | ngx-drag-to-select Version |
| -------------------- | -------------------------- |
| 5.x.x | 1.1.1 |
| 6.x.x - 7.x.x | 3.1.1 |
Copy link
Owner

Choose a reason for hiding this comment

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

Same here => > 1.1.1 <= 3.1.1

Copy link
Owner

Choose a reason for hiding this comment

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

And also add a new row with

>= 8.x.x              >= 4.0.0

@cyclist82
Copy link
Contributor Author

Thanks again for the nice feedback and your help to properly document the change in the docs.

I added the proposed changes to the README. Please have another look @d3lm.

Copy link
Owner

@d3lm d3lm left a comment

Choose a reason for hiding this comment

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

Last remarks, promised 😂 👍

README.md Outdated

You can still install it via npm or yarn, e.g. `npm install ngx-drag-to-select@x.x.x`.

| Angular Version | ngx-drag-to-select Version |
Copy link
Owner

Choose a reason for hiding this comment

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

Love it!!

README.md Outdated

The latest version that supports Angular 5.x is 1.1.1. You can still install it via npm or yarn, e.g. `npm install ngx-drag-to-select@1.1.1`.
Yep you totally can! But this also means you won't be able to use the latest and greatest and all of the newer features. The reason for this is that we are not back porting new features to older versions of this library, due to maintenance overhead. Severe bugs and security issues, however, are back ported.
Copy link
Owner

Choose a reason for hiding this comment

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

Super tiny update. Let's say

due to additional maintenance.

README.md Outdated

You can still install it via npm or yarn, e.g. `npm install ngx-drag-to-select@x.x.x`.

| Angular Version | ngx-drag-to-select Version |
Copy link
Owner

Choose a reason for hiding this comment

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

Let's change the headers to only Angular and ngx-drag-to-select. The "version" can be inferred by the user very quickly by looking at the rows.

Copy link
Owner

@d3lm d3lm left a comment

Choose a reason for hiding this comment

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

This looks good to me! Well done and thanks for your work 🙏

@d3lm d3lm merged commit 5b490f1 into d3lm:master Oct 28, 2020
This was referenced Oct 30, 2020
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

2 participants