Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

RFC: drop support for primitive shorthand values #2206

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Jan 3, 2020

RFC: drop support for primitive and array shorthand values

The proposal here is to drop the support for using primitive and array shorthand values:

<Button icon="book" />
<Menu items={["Editorials", "Reviews", "More"]} />

to be replaced with:

<Button icon={{ name: "book" }} />
<Menu items={{items: ["Editorials", "Reviews", "More"]}} />

*The second option is already available

Why?

  1. We will have only way of defining shorthands, so customers will be able to easily understand and apply it

  2. Currently, customers don't understand the transition from primitive shorthands to props object. For example, why in the case of the icon prop, the primitive is mapped to name, but in the case of content, it is mapped to content

<Button icon="book" content="Click me" />

is the same with

<Button icon={{name: "book"}} content={{content: "Click me"}} />

For the problem with content={{content: '...' }}, @layershifter already has an RFC for removing it: #2205

  1. There is a confusion, as people see only simple examples in the components' docs page and don't understand that they can pass additional properties :(

  2. It will greatly simplify the internal implementation - we will be able to remove lots of the logic we have now where we have to handle primitives + objects, and in the future will allow us to have more generic way of internal injection of utilities classes/properties to the components, without having to do some checks for primitives or figuring out which property the primitive value is mapped to. (this will reduce the number of potential bugs we may introduce...)

Problems:

  1. Of course this will mean lots of breaking changes for the customers. Typings should help with this, but there will be cases where people are just spreading props on the componentS, so it may be hard to track all places that should be changed...

Please share your opinion!

@ecraig12345
Copy link
Member

From the perspective of convergence and making typings cleaner, I like this. (Can't speak to the existing consumer perspective of course.)

@kamilakorzec
Copy link

From the performance perspective, this will force re-render every component when the parent re-renders if you don't provide some comparison mechanism for components that consumed primitive values till now. Is there any plan to mitigate it?

@johannao76
Copy link
Contributor

johannao76 commented Jan 22, 2020

What was the answer to Kamila's question above? We should ensure we do proper performance testing prior to accepting this change. Similarly are there any memory concerns as now I see we will be creating more objects? In a smaller scale, this may be ok but in the large scale of our app, could this be an issue?

@mnajdova
Copy link
Contributor Author

@kamilakorzec @johannao76 Thank you for the considerations. It will not have significant impact of performance or memory because we are already in more than 70% using the shorthand object. We are actually more interested to hear the opinion on the API changes themself from client's perspective, so please share if you have some doubts or suggestions around that. Thank you!

@kohlikohl
Copy link
Collaborator

I understand the motivation for simplifying the shorthands, however, I think for a user that is just interested in simple modifications such as the API is much cleaner currently.

Is the issue here just that users don't understand how to "upgrade" from a simple use case to a complex use case?

  1. Could that be solved through documentation?
  2. Is the removal of the simple shorthand API increase performance etc?
  3. I understand that it would be easier to maintain the code, but is that in the best interest of the consumer?
  4. Also, do we know how users mostly use the API?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants