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

Elements should know if the spread operator is used #4272

Closed
jimfb opened this issue Jul 2, 2015 · 6 comments
Closed

Elements should know if the spread operator is used #4272

jimfb opened this issue Jul 2, 2015 · 6 comments

Comments

@jimfb
Copy link
Contributor

jimfb commented Jul 2, 2015

The only bug number that I have memorized at this point is #140. It keeps coming up, and maintaining the ever-growing whitelist is infuriating.

As a baby step toward fixing that bug, I think it would be nice to know if the spread operator is used during element creation, since this is the primary way that attributes could be accidentally passed to an element. We could then only enforce the whitelist on elements that are using the spread operator, as a baby step toward eliminating the whitelist altogether. In order to do this though, I think we need to modify the transform (Babel?), right? Do we want to file a bug against them?

This issue is mostly exploratory, to get an idea of what would need to be involved to do this effectively. RFC: @spicyj, @sebmarkbage

@syranide
Copy link
Contributor

syranide commented Jul 3, 2015

Huh... modifying the spread operator like this makes no sense at all to me, JSX isn't even HTMLDOM-specific and not everyone uses it.

I only see three possibilities that makes sense right now:

  1. Keep the white-list and add new prop attrs={{cust: ''}} or attr-cust=""
  2. Keep the white-list and pass-through any props not on it as-is
  3. Drop the white-list and pass-through all props as-is

Or what am I missing?

@sophiebits
Copy link
Collaborator

Also negative feelings on this. The whole point of object spread is that it works the same as not spreading.

@sebmarkbage
Copy link
Collaborator

There are other ways to get prop typos in. Explicit typos, intentional, Object.assign, object spread.

What's the progression and end goal here? Are you going to warn for explicit typos first? And then tackle the spread or are spread always going to remain unwarned? What's the point of warning for non-spread before warning for spread?

@jimfb
Copy link
Contributor Author

jimfb commented Jul 6, 2015

@sebmarkbage Object.assign and object-spread will ultimately manifest themselves as a jsx-spread, since that's how you go from an object-full-of-props to an element with properties, so those use cases are covered (assuming the person is using jsx).

My working assumption is that if a person does <div {...myProps} />, the person might be relying on the whitelist to filter props, but if they are explicitly setting props (eg. <div align='left' />) then they aren't just forwarding props and so the whitelist filtering is providing very little value.

This is a good first step because it unblocks anyone using custom attributes (They now have options: can create the element without using the spread, or they can invoke React.createElement directly), while sidestepping the main blocker of #140 (namely, people using spread to forward props that they don't intend to be inserted into the DOM because they are relying on the whitelist).

It solves the immediate issue at hand (no way to pass custom attributes to a DOM element) without being blocked by use of the spread operator, and it allows us to break the migration into two steps (fix everything except spreads, and someday in the future fix the spreads too allowing us to remove the whitelist completely).

@syranide Correct, it isn't htmldom specific and not everyone uses it. People who don't use it would need to migrate their code to not rely on the whitelist (we provide warnings to help them with this). As for it not being htmldom specific - that's true, but we're making the situation strictly better than it was before (for both htmldom and non-htmldom), so I'm ok with this.

@sophiebits
Copy link
Collaborator

I don't think it's a big issue that we'll pass down props too much in some cases. I'd expect at most one or two things to break in FB code and maybe another one or two for external users. More importantly, it doesn't seem like we'd have any other good path to weed those out before a switch (unless we can figure out which attributes have meaning but are currently not passed so that they can be logged), so I think we may as well switch all at once.

@gaearon
Copy link
Collaborator

gaearon commented Oct 27, 2016

Closing as no longer relevant.

@gaearon gaearon closed this as completed Oct 27, 2016
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

No branches or pull requests

5 participants