Skip to content

Conversation

jquense
Copy link
Collaborator

@jquense jquense commented Nov 22, 2016

Moving code over from fb/react

I’m not entirely sure how to handle licensing in this case..

@jquense
Copy link
Collaborator Author

jquense commented Nov 22, 2016

There where a few (a lot) of internal api usage, most was easy enough to fix

  • dom-helpers instead of CSSCore, we use it on react-bootstrap so probably a safe swap.
  • Remove the transition event code branch entirely (it was already deprecated) and depend on timeouts. we can always use the dom-helpers version if we want to add it back
  • Do some really hacky stuff with Children to avoid needing flattenChildren

@jquense
Copy link
Collaborator Author

jquense commented Nov 22, 2016

cc @gaearon

Children.map(children, child => child)
.forEach((child) => {
result[child.key] = child;
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is preeeety nasty. @gaearon do you have any other suggestions as a replacement for the internal flattenChildren that was originally used here?

Copy link
Member

Choose a reason for hiding this comment

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

Does something like Children.toArray(children) help? I don't actually know this part of the code well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, or at least not fully.

from what I gather, flattenChildren will return object where the keys are the react key/data-id and values the corresponding children. my code above is attempting to emulate it by running it through Children.map which flattens and adds keys to unkeyed elements. The forEach turns it into a hash.

The double iteration isn't great, but also we lose the extra debug info that flattenChildren gave, pointing out duplicates keys.

@gaearon
Copy link
Member

gaearon commented Nov 22, 2016

cc @zpao, how should we handle LICENSE/PATENTS files and copyright headers?

@taion
Copy link
Member

taion commented Nov 22, 2016

Wasn't there a question a bit ago about support for raw numbers between CSSCore and dom-helpers? Or is that not applicable here?

@gaearon
Copy link
Member

gaearon commented Nov 22, 2016

Remove the transition event code branch entirely (it was already deprecated) and depend on timeouts. we can always use the dom-helpers version if we want to add it back

Can we keep it for one version? So that initial release is functionally identical to that provided by React 15. Then we can cut 2.x that drops it.

@jquense
Copy link
Collaborator Author

jquense commented Nov 22, 2016

Can we keep it for one version? So that initial release is functionally identical to that provided by React 15. Then we can cut 2.x that drops it.

sure, no problem

@jquense
Copy link
Collaborator Author

jquense commented Nov 22, 2016

Wasn't there a question a bit ago about support for raw numbers between CSSCore and dom-helpers? Or is that not applicable here?

yes but in case its just using the className utils, not styles

@gaearon
Copy link
Member

gaearon commented Dec 8, 2016

Let's do it 👍

@jquense
Copy link
Collaborator Author

jquense commented Dec 9, 2016

ok last bit added 👍

@jquense jquense merged commit 02c4c0e into master Dec 9, 2016
@gaearon gaearon deleted the fork branch December 9, 2016 15:36
@jquense
Copy link
Collaborator Author

jquense commented Dec 9, 2016

ok released! If all want to get the word out for folks to try it out, to make sure I didn't break anything in the move.

@gaearon
Copy link
Member

gaearon commented Dec 9, 2016

Do you mind forking the docs too?

@gaearon
Copy link
Member

gaearon commented Dec 9, 2016

Docs have a separate license: https://github.com/facebook/react/blob/master/LICENSE-docs

@jquense
Copy link
Collaborator Author

jquense commented Dec 9, 2016

yes eventually, but a bit short on time right now! For now though, i don't think linking to the original docs places should be licensing issue right?

@gaearon
Copy link
Member

gaearon commented Dec 9, 2016

Sure it's fine. I just mean in case you want to make API changes or anything.

Also the docs refer to React.addons so beginners will likely not realize they need to use a different package.

@jquense
Copy link
Collaborator Author

jquense commented Dec 9, 2016

yeah its not a great solution in the medium to long term. We'll get some proper docs up on a gh-pages soon as I have some time to fork and extract the current ones

@gaearon
Copy link
Member

gaearon commented Dec 9, 2016

I think it's also fine just to copy & paste this on README and leave it on GitHub.
You'll miss out on our custom line highlighting but otherwise it's okay.

@jquense
Copy link
Collaborator Author

jquense commented Dec 9, 2016

ha true, I forgot it's all markdown :P

@jquense
Copy link
Collaborator Author

jquense commented Sep 5, 2019

🎉 This issue has been resolved in version 4.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@pxwee5 pxwee5 mentioned this pull request Sep 2, 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.

3 participants