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

Deprecate the Input component usage in AuInput #250

Closed
Windvis opened this issue May 10, 2022 · 9 comments · Fixed by #429
Closed

Deprecate the Input component usage in AuInput #250

Windvis opened this issue May 10, 2022 · 9 comments · Fixed by #429

Comments

@Windvis
Copy link
Contributor

Windvis commented May 10, 2022

AuInput is one of the last components that still uses Input's 2-way-binding internally. This has the downside that it's not suitable for all use-cases and why some apps have to resort to using native inputs with css classes instead.

It would be better to stop using Input and let consumers update the value with a change handler. The preference is to do this with the {{on}} modifier instead of a custom @onChange action since you have more flexibility over the used event (with the downside that the value needs to be read from the event object).

Since this will have a major impact on all apps we can't expect them to change it all at once so a build-time only flag isn't a good option here. We do need a way to detect when the user wants the native element instead of the 2-way-bound Input.

Some options:

  • add a @useNativeElement argument (or similar) which we can use to swap out the implementation.
  • deprecate @value and push users to the attribute instead? in that case it makes sense to also deprecate some of the other Input arguments (@type, @id) and let users pass these in as attributes as well.
  • Deprecate the whole AuInput component and create a new AuNativeInput component instead
@nvdk
Copy link
Collaborator

nvdk commented May 16, 2022

might make (more) sense to provide a separate AuNativeInput component and deprecate the AuInput component?

@Windvis
Copy link
Contributor Author

Windvis commented May 16, 2022

@nvdk It's an option, but it will have the same end-result as the other options but with a longer name 😄. Any reason why you prefer that route?

@nvdk
Copy link
Collaborator

nvdk commented May 16, 2022

I think it'll be easier to spot what type of input you're using in the app itself. I image a lot of apps will have mixed usage for a while.

@nvdk
Copy link
Collaborator

nvdk commented May 16, 2022

I assume it also makes deprecation checks a bit easier in appuniversum itself, though that was less of concern for me

@Windvis
Copy link
Contributor Author

Windvis commented May 16, 2022

I think it'll be easier to spot what type of input you're using in the app itself. I image a lot of apps will have mixed usage for a while.

Yes, that's true, but the @useNativeElement option provides similar benefits I think? It should also be relatively simple to resolve the deprecations so you could switch to the new setup fairly fast I think.

Anyway, I prefer to keep the AuInput name since it's shorter and is the most intuitive name for such a component but I'm not opposed to creating a new one if that's the consensus. I'll add it to the list 👍

@abeforgit
Copy link
Contributor

Hit this again today, just to notify that it's still a desired change

@Windvis
Copy link
Contributor Author

Windvis commented Jan 6, 2023

@abeforgit Any preference on one of the implementation options? 😄

@abeforgit
Copy link
Contributor

agree that AuNativeInput is a clumsy name for what should be the only version of the component
I like breaking stuff so I'd probably just release as a new major and call it a day
but if we're being nice, I suppose deprecating the arguments makes the most sense? The extra argument means that consumers that "do the right thing" immediately will be punished by needing another refactor for when that argument gets removed again

but its a small preference

@Windvis
Copy link
Contributor Author

Windvis commented Sep 13, 2023

I went with option 2: #429

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 a pull request may close this issue.

3 participants