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.

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

RFC: refactor to one class with multiple implementable interfaces #144

Closed
zakhenry opened this issue Feb 22, 2020 · 8 comments · Fixed by #188
Closed

RFC: refactor to one class with multiple implementable interfaces #144

zakhenry opened this issue Feb 22, 2020 · 8 comments · Fixed by #188
Assignees
@zakhenry @maxime1992
Labels
effort-3: days Will take one day or more to fix/create flag: breaking change This feature or fix will require a breaking change released on @feat-rewrite released scope: lib Anything related to the library itself state: has PR A PR is available for that issue type: RFC/discussion/question This issue is about RFC, discussion or a question
Milestone
v6.0

Comments

@zakhenry
Copy link
Contributor

Currently we have a bit of a proliferation of classes that can be inherited, and potentially more to be introduced with @maxime1992’s idea of enforcing better typesafety on non-remap components.

I think now with the more expressive typescript conditionals we can be much more clever with interfaces and go for a composition model where the consumer class might need the base class only with a single interface in and out, or need a combination of a bunch of different features that ngxsubform would need to access, which could be declared and made typesafe through multiple implementable interfaces.

This would be a major breaking change, and we should consider an angular style migration to auto update consumer classes if possible.

I’ve made some progress on the types here:

https://www.typescriptlang.org/play/?ssl=1&ssc=1&pln=55&pc=13#code/JYOwLgpgTgZghgYwgAgEoQLZwA4GED2G2+IE4APAeFPgDYCS408SANMgGL5QaOSyIIAPmQBvALAAoZMizYAFADc4tAFzIqYGgyYCkASnVcefZoIDcUgL5SpYAJ7YUAUQAeWxGGO9dLCOQAVdlwRAF5kAOQIdzIAEwBnNEwcAiISMjBKdlAYaE4RAH5OZHVcS0kpBFo4eMSAOQBzVwBlAFcAI29U4lIKTW1TPWExKRkGiC9uDAA1FVaIeUNkNw8ESZNfQXIwAAtgeOCSLTpBvxEJaRlkKAnWqBBkVpBYiBhQCFjkGq+Qe3KZGwVS77dByRbqXb7ZBQ0EpQg9DLkOC-djI+znUZXG5gO4PeC0eIob5o-7IQGYsD4fonTZIeSYmTKNTLdxQTzeU5bSEHDRHAa04SYpbUnT8PwjS4yYAwZDybkAOhByQU+n0EquWNu92QCrBTP0pIBtkl1y1DyZpMB5MkoDFgmQAFl8C9RWYkOrkDB8Ph1PEtKAGuVrba3SgOQKPe04FBff6QIHrMaqjVEs1gERaBAuvD0uAojFnvUmm1OlNurnMk6XZykOdrcnakk5Nm0r0wPnIIXkI0Wh0WwiKFWIK6hkJMensJmMBlErC8Dm2+QhyO-Oxw3bax69Sp1MuaxAluvQx6ZNjcWJkFGY8gAEQ7G9ky2JoEIEh+2Q4JyfcKkADuyDTDMszLBcMkWeVKRFfd5FET1vXUG84AfKwDUqN92zkL8ACZkB-CB-znfsK3AyC+RpDcFlgq8EKQslUJfdCOzZNZcOQP8AInTMiLbcDxnWGY5gWejXxAd9olWMAcLwgjlW4sD9HlPjvFmWh5kWcopHE5iwHlL18HKLTPCw+Ur3KIA
@zakhenry zakhenry added scope: lib Anything related to the library itself effort-3: days Will take one day or more to fix/create flag: breaking change This feature or fix will require a breaking change state: needs design This feature or fix should be discussed before writing any code type: RFC/discussion/question This issue is about RFC, discussion or a question labels Feb 22, 2020
@zakhenry zakhenry added this to the v3.0 milestone Feb 22, 2020
@zakhenry zakhenry self-assigned this Feb 22, 2020
@zakhenry
Copy link
Contributor Author

Further progress, I had a go at doing the refactor, but got stuck on a particular type, here's a cut down example of the problem:

https://www.typescriptlang.org/play/?ssl=47&ssc=46&pln=47&pc=60#code/JYOwLgpgTgZghgYwgAgEoQLZwA4GED2G2+IE4APAeFPgDYCS408SANMgGL5QaOSyIIAPmQBvALAAoZMizYAFADc4tAFzIqYGgyYCkASnVcefZoIDcUgL5SpofixQBZOCACuKgMpxFKCdNlXD1pvX3UtNwhLSRtJKTAAT2wUAFEADy1EMGNeXUdyABV2XBEAXmQC5AgMsgATAGc0TBwCIhIyMEp2UBhoThEAfk5kdVxo+KSUABFoYF9NbVM9CEKyiqqakAbkADkAczTPNwAjHNbiUgoevpLkIdwR5FJfKGjE5OQZqDmIHKX8gprSrVSBbRr7Q4nM6EC4dcjXKAaQbrEF1RroOTndoUXDdEC9REcZEcR4PdTPaDjOKSGBuEAIMDAEjIYD1DE4LoozbbCFHU7cDBYy6dEpCeQIGHYsDqAqGZAStrClmNSoAMiamMlws56UyDL+eUEhWKQhE-hkUAgYDcUBAyAAhPaFbDwAA6AAWcHqAHkAO4gAAKNGSUES8gA5HJw-porEpHBjvU9WB5bQveCDnzoYq4Qs6P9BGbbAEE0moFlkHsrTkAGoqSLyOW68v6gUFpDkMDu1nFEhafOGpBCKkyVnshRyrus5UalpauGuBLsRdFgIWq02u3wWj1FBe5CL6IyOMBMD4PM6ByCeRSGQyZRqZDNrIGq8dqf1XvUAdv4S35Byhe7Z+P+o4wMg8gfq6Y7NBO+hiKBd6WtatrIFBchKCoMaISed7IMhm7IA+R7ILEJ72GYSDIE4+C1BAl6USBAQwPg+DqGWoB7LGxYUcsnBtoOTEyMccBQOxWicdx1IIGm9SNJ4wBELQvwCkKHQbKCPKZlCqnzhQNF0QxyyrjIVbZAKda0A2TYZC25kmIJnbdp+1G0fRwEmUhG6oaIyAsWxTxuLQtCkf+uF+axjYIWu8okEmB7qF8PxAY5H5rOaeERQF4ZwOGOEjneEogPUdAQK6tD4Hs8hwK62EBCeJ4yems7YNmLopqiYK7Np-I8Gp+luUZjhCP+inYMpGAdOisH9Z0BnuYJ7CvoxQjsC47heD4TH-pacC1CQtAJIEG0hFtyDlBEUTFqZ1YWfWECNuoz6tg5v5OT2rmGR50V4QRqF0nRMCgBAtQHo0h5hddgQKA+6jzUNghystfEZeuKF2r5IlicgABE7o46RJFkcWRXxXIySg+UpC+sgClKSpfV6WAjaumeKW-vIvn+eoONwATVh1aTKbkyDABM51PBANPjm1Uos2zfaLIJnPIFjPN86RdVSELGl2RL1O02Nymy8KLNmbW92NtEOsgnZ4tU1LLUmx0Zu3TwlnWVSttZK6-nRN7DKi66WPmEAA

@zakhenry
Copy link
Contributor Author

Posted to stackoverflow with a more general example: https://stackoverflow.com/questions/60365193/typescript-how-to-correctly-type-a-member-of-abstract-generic-class-based-on-p

@zakhenry
Copy link
Contributor Author

refactor/composition-of-interfaces-over-inheritance created, WIP and broken

@maxime1992
Copy link
Contributor

Just to link related issues, this would solve #133

@maxime1992 maxime1992 mentioned this issue Apr 23, 2020
Closed
@maxime1992 maxime1992 mentioned this issue May 6, 2020
Closed
maxime1992 added a commit that referenced this issue Jun 15, 2020
@maxime1992
feat: rewrite of ngx-sub-form without inheritance
719d640 
This is a major architecture change which is brought without any breaking change 😄!

We've split up the code base in 2: Old one and new one.
The old one hasn't moved at all but is now deprecated (not removed yet!).
You can keep using the old one for a bit and have a smooth/incremental update to use the new API.

Few changes that you have to note with the new API:
- Only works with Angular 9 or more
- The app needs to have Ivy activated (this is because we use `ɵmarkDirty` internally. If it ever gets removed we'll probably have to ask to provide the `ChangeDetectorRef` but we were able to around this for now!)
- We got rid of inheritance 🙌
- Form errors on a FormArray are now an object instead of an array. Previously the array contained null values on all the fields without any error. It's now an object containing only the ones with errors and you can access them using the index

Please start upgrading to the new API as soon as possible as we stop supporting the old API as of today and will remove it in a near release.

This closes #171 for the major architectural changes and also the following issues as a result:
- closes #82
- closes #86
- closes #93
- closes #133
- closes #143
- closes #144
- closes #149
- closes #160
- closes #168
@maxime1992 maxime1992 added state: has PR A PR is available for that issue and removed state: needs design This feature or fix should be discussed before writing any code labels Jun 15, 2020
@maxime1992 maxime1992 self-assigned this Jun 15, 2020
@maxime1992
Copy link
Contributor

🎉 This issue has been resolved in version 5.2.0-feat-rewrite.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@maxime1992 maxime1992 linked a pull request Oct 11, 2020 that will close this issue
feat: rewrite of ngx-sub-form without inheritance #188
Merged
@maxime1992
Copy link
Contributor

@zakhenry I think this will be made obsolete by the rewrite and I've marked this issue to be closed once #188 is merged. If you believe this is a mistake let me know but I don't think so :)

@zakhenry
Copy link
Contributor Author

Agreed

zakhenry pushed a commit that referenced this issue Oct 23, 2020
@maxime1992 @zak-cloudnc
feat: rewrite of ngx-sub-form without inheritance
8d3400c 
This is a major architecture change which is brought without any breaking change 😄!

We've split up the code base in 2: Old one and new one.
The old one hasn't moved at all but is now deprecated (not removed yet!).
You can keep using the old one for a bit and have a smooth/incremental update to use the new API.

Few changes that you have to note with the new API:
- Only works with Angular 9 or more
- The app needs to have Ivy activated (this is because we use `ɵmarkDirty` internally. If it ever gets removed we'll probably have to ask to provide the `ChangeDetectorRef` but we were able to around this for now!)
- We got rid of inheritance 🙌
- Form errors on a FormArray are now an object instead of an array. Previously the array contained null values on all the fields without any error. It's now an object containing only the ones with errors and you can access them using the index

Please start upgrading to the new API as soon as possible as we stop supporting the old API as of today and will remove it in a near release.

This closes #171 for the major architectural changes and also the following issues as a result:
- closes #82
- closes #86
- closes #93
- closes #133
- closes #143
- closes #144
- closes #149
- closes #160
- closes #168
maxime1992 added a commit that referenced this issue Nov 21, 2021
@maxime1992
feat: rewrite of ngx-sub-form without inheritance
fef8cce 
This is a major architecture change which is brought without any breaking change 😄!

We've split up the code base in 2: Old one and new one.
The old one hasn't moved at all but is now deprecated (not removed yet!).
You can keep using the old one for a bit and have a smooth/incremental update to use the new API.

Few changes that you have to note with the new API:
- Only works with Angular 9 or more
- The app needs to have Ivy activated (this is because we use `ɵmarkDirty` internally. If it ever gets removed we'll probably have to ask to provide the `ChangeDetectorRef` but we were able to around this for now!)
- We got rid of inheritance 🙌
- Form errors on a FormArray are now an object instead of an array. Previously the array contained null values on all the fields without any error. It's now an object containing only the ones with errors and you can access them using the index

Please start upgrading to the new API as soon as possible as we stop supporting the old API as of today and will remove it in a near release.

This closes #171 for the major architectural changes and also the following issues as a result:
- closes #82
- closes #86
- closes #93
- closes #133
- closes #143
- closes #144
- closes #149
- closes #160
- closes #168
@github-actions
Copy link

🎉 This issue has been resolved in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zakhenry zakhenry

@maxime1992 maxime1992

Labels
effort-3: days Will take one day or more to fix/create flag: breaking change This feature or fix will require a breaking change released on @feat-rewrite released scope: lib Anything related to the library itself state: has PR A PR is available for that issue type: RFC/discussion/question This issue is about RFC, discussion or a question
Projects
None yet
Milestone
v6.0
Development

Successfully merging a pull request may close this issue.

feat: rewrite of ngx-sub-form without inheritance cloudnc/ngx-sub-form
2 participants
@zakhenry @maxime1992