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

[Fiber] Investigation: what internals have direct dependencies on Stack modules? #9069

Closed
trueadm opened this issue Feb 27, 2017 · 1 comment
Assignees

Comments

@trueadm
Copy link
Contributor

trueadm commented Feb 27, 2017

After some research, the following modules in the React codebase have some form of direct dependency on a React Stack module. Ideally, we want to move away form using internal module requires and use public API requires where possible.

ReactTestUtils:

Renderers:

  • src/renderers/__tests__/ReactUpdates-test.js
    • Only one test makes use of ReactUpdates directly. Move the require to be inline within the test itself?
      @spicyj says: this one just tests internals and doesn't test anything observable (originally was added as a part of a very minor perf improvement), can be deleted

Renderers - ReactDOM:

  • src/renderers/dom/shared/__tests__/ReactMount-test.js
    • A test in this suite uses ReactMount._instancesByReactRootID.
      @spicyj says: this test was added because the old devtools relied on this and it was important to not break them -- but Dan rewrote the integration so the Fiber devtools don't need this and this test is fine to delete
  • src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
    • Two tests: createOpenTagMarkup and createContentMarkup require Stack modules.
      @spicyj says: these tests should be rewritten in terms of public APIs
  • src/renderers/dom/shared/__tests__/ReactServerRendering-test.js
    • Re-visit once we have SSR working?
      @spicyj says: 👍
  • src/renderers/dom/shared/__tests__/ReactDOMIDOperations-test.js
    • Only has a single innerHTML whitespace test.
      @spicyj says: let's rewrite this one in terms of public APIs (though it can probably move into ReactDOMComponent-test) -- let's ensure that both initial render and updates work properly (since those historically used different codepaths and might also diverge in the future)

Renderers - ReactDOM SSR:

  • src/renderers/dom/ReactDOMServer.js
    • Re-visit once we have SSR working?
      @spicyj says: 👍

Renderers - ReactART:

  • src/renderers/art/ReactARTStack.js
    • A couple of Stack modules are required by ART.
      @spicyj says: No need to worry about these, we'll delete these when we delete stack
      @bvaughn says: 👍

Renderers - Native:

  • src/renderers/native/ReactNativeReconcileTransaction.js
  • src/renderers/native/ReactNativeMount.js
  • src/renderers/native/ReactNativeStackInjection.js
  • src/renderers/native/ReactNativeBaseComponent.js
  • src/renderers/native/ReactNativeStack.js

@spicyj says: No need to worry about these, we'll delete these when we delete stack
@bvaughn says: 👍

Scripts:

  • scripts/bench/extract-component.js
    • Should we refactor to use the public API instead?
      @spicyj says: We don't really have a public API for reflecting on/inspecting React trees which is what this script (almost a bookmarklet) does so we will need to rewrite this on top of Fiber if we want to use it again – but this isn't super important so for now this is okay to leave using Stack modules and it will only work with Stack until someone wants to use it and upgrades it to Fiber.
@sophiebits
Copy link
Collaborator

I added comments inline, summary looks good assuming you didn't miss anything. :)

Only a couple tests that need to be rewritten and I expect them to pass out of the gate with Fiber enabled since they're not anything we haven't implemented. Mind rewriting those and sending a PR?

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

No branches or pull requests

7 participants