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

Create Separate Copies of Each Renderer #6795

Closed
sebmarkbage opened this issue May 18, 2016 · 10 comments
Closed

Create Separate Copies of Each Renderer #6795

sebmarkbage opened this issue May 18, 2016 · 10 comments

Comments

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented May 18, 2016

A simple plan:

  1. Copy all files to each renderer package.

  2. Rewrite any require to the files in the isomorphic folder to require('react/lib/MyFileName'). (Hardcode a whitelist if needed.)

That way they all share isomorphic modules but none of the other files.

That way versioning separate renderers is easy and doesn't depend on everyone updating their react peer dependencies.

@iamdustan
Copy link
Contributor

Will the renderers be:

  • ReactDOM
  • ReactDOM/server
  • ReactNative
  • ReactART

This plan is regarding mapping haste to node_modules as a build step, correct?

@zpao
Copy link
Member

zpao commented May 18, 2016

Renderers are still dependent on the react package right? We can just version them independently without having to publish React (in other words, what we've wanted to do for a while), and theoretically just stick their dep as react: ^15.0.0?. We might still want to version react-dom with react, at least initially, but it would help immediately with the react-native-renderer case.

Do we think this lasts longer term, particularly around the current owner? I know we talked about pulling that out as well (and @IamDustin made that effort)

@sebmarkbage
Copy link
Collaborator Author

Yea, this was the plan all along.

The current owner is part of react and for now that is a peerDependency so it should just work. If it was pulled out, then react doesn't necessarily need to be a peerDependency anymore and we could support multiple versions of React but that's not as critical.

ReactART won't work as its own standalone yet. It'll have to depend on ReactDOM, but hopefully I can cut it over to the new reconciler.

@zpao
Copy link
Member

zpao commented May 18, 2016

This plan is regarding mapping haste to node_modules as a build step, correct?

Right, and then eventually cut out the haste part entirely (#6336).

Yea, this was the plan all along.

Cool, just making sure this wasn't some variation.

@sebmarkbage
Copy link
Collaborator Author

Cool, just making sure this wasn't some variation.

The variation is that this doesn't properly remove unused files from the packages, and it still doesn't do flat builds. So it is an uglier subset I guess.

@zpao
Copy link
Member

zpao commented May 18, 2016

If we're already needing to track which requires are from isomorphic, I think we can skip copying those files over to the renderer packages pretty easily (and subsequently the inverse).

Edit: ultimately the file exclusion doesn't matter since they won't be referenced by the packages themselves. And maybe it makes sense to not do any exclusion short-term, the addons packages might get a bit trickier…

@sophiebits
Copy link
Collaborator

Yeah, I agree with the goal here. We don't need to go all the way though. The simplest near-term solution looks to me to be copying the native renderer files into RN when we want to update and leaving React DOM alone for now. That solves our versioning problems here and also solves our internal problem where RN devs are finding it frustrating to work with RN renderer files in the react package. Then RN will depend on react only for the isomorphic bits.

@sebmarkbage
Copy link
Collaborator Author

That would further delay getting rid of providesModule, flat bundles with a build step, and might necessitate keeping specialized flow configuration since flow will special case node_modules. And whatever other non-idiomatic thing we'll be chasing to support this instead of addressing the core issue.

I'm fine with this but I feel like we never permanently fix anything anymore. We just do the minimal viable thing so we can fix the next thing next week.

On May 18, 2016, at 4:43 PM, Ben Alpert notifications@github.com wrote:

Yeah, I agree with the goal here. We don't need to go all the way though. The simplest near-term solution looks to me to be copying the native renderer files into RN when we want to update and leaving React DOM alone for now. That solves our versioning problems here and also solves our internal problem where RN devs are finding it frustrating to work with RN renderer files in the react package. Then RN will depend on react only for the isomorphic bits.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@sebmarkbage
Copy link
Collaborator Author

Once you have to make a build script that does this, why not just make it do both packages at the same time since they currently line up to do the same thing?

We can copy the result into the RN repo or not but seems orthogonal.

The alternative is to put the sync and build script into the RN repo but we still have to build it and we'd do so in a way that doesn't address both use cases.

I guess I don't see why building once for both is harder than one time for RN-only (and then another time later).

@gaearon
Copy link
Collaborator

gaearon commented Apr 4, 2017

I guess it was done in 15.4.0.

@gaearon gaearon closed this as completed Apr 4, 2017
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