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

Drop Haste #11303

Merged
merged 29 commits into from Oct 24, 2017

Conversation

Projects
None yet
4 participants
@gaearon
Member

gaearon commented Oct 20, 2017

TODO

  • Fix Flow imports
  • Fix build
    • Fix UMD
    • Fix CJS
    • Fix FB
    • Fix RN
    • Fix size regressions
      • Fix DEV-only regression
  • Fix error extraction
  • Remove Jest providesModules things

Not sure how I feel about this.

This helps enforce that we have proper module boundaries between packages. It already exposes some renderers read directly from reconciler (and thus won't be accessible to third party renderers). So I think that is kind of nice.

This exposed an interesting issue with Jest and Yarn Workspaces (facebook/jest#3830).

In a test, ../createReactNativeComponentClass and require('react-native-renderer').__SECRET_INTERNALS_DO_NOT_USE.createReactNativeComponentClass will be two different things. Because one is imported relatively (and thus is "located" in packages) while the other one is instantiated from code path going via node_modules/react-renderer. Jest thinks they are different.

This seemed like a show stopper to me at first. But then I thought, we don't like tests that do ../ anyway because they read internal modules. What if we just outlawed it?

In this case, you'd have to write react-native-renderer/src/createReactNativeComponentClass to get it in a test. Or read it from a secret bag exposed on react-native-renderer. But that might actually be okay because it makes "bad" code looks bad. All existing cases of this already have TODOs next to them (#11299). So we could forbid relative requires in tests. Thus also encouraging people to write integration tests for new features too.

We could also wait for Jest to fix it but I doubt that will happen soon since almost nobody else bumped into this.

Mostly seeking feedback if anyone has strong objections at this point. If not, I could further flatten the structure, forbid relative requires in tests, and then fix the build. I expect that Rollup configuration will be significantly simpler now that we can let it use Node resolving mechanism without silly whitelists like:

// these are the FBJS modules that are used throughout our bundles
const fbjsModules = [
'fbjs/lib/warning',
'fbjs/lib/invariant',
'fbjs/lib/emptyFunction',
'fbjs/lib/emptyObject',
'fbjs/lib/hyphenateStyleName',
'fbjs/lib/getUnboundedScrollPosition',
'fbjs/lib/camelizeStyleName',
'fbjs/lib/containsNode',
'fbjs/lib/shallowEqual',
'fbjs/lib/getActiveElement',
'fbjs/lib/focusNode',
'fbjs/lib/EventListener',
'fbjs/lib/memoizeStringOnly',
'fbjs/lib/ExecutionEnvironment',
'fbjs/lib/createNodesFromMarkup',
'fbjs/lib/performanceNow',
];

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Oct 21, 2017

Member

Not sure how I feel about this.

...goes on to list only benefits.

Member

sebmarkbage commented Oct 21, 2017

Not sure how I feel about this.

...goes on to list only benefits.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 21, 2017

Member

I'll write something more comprehensive when I get it working :-)

Member

gaearon commented Oct 21, 2017

I'll write something more comprehensive when I get it working :-)

@gaearon gaearon changed the title from [RFC] Drop Haste to Drop Haste Oct 24, 2017

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 24, 2017

Member

I don't understand what CI failure means. It builds locally. But on the server require.resolve('react') doesn't find anything. I expect it to resolve to node_modules/react per symlink. I cleared CI cache a few times.

Member

gaearon commented Oct 24, 2017

I don't understand what CI failure means. It builds locally. But on the server require.resolve('react') doesn't find anything. I expect it to resolve to node_modules/react per symlink. I cleared CI cache a few times.

gaearon added some commits Oct 20, 2017

Fix as many issues as I can
This uncovered an interesting problem where ./b from package/src/a would resolve to a different instantiation of package/src/b in Jest.

Either this is a showstopper or we can solve it by completely fobbidding remaining /src/.
Fix all tests
It seems we can't use relative requires in tests anymore. Otherwise Jest becomes confused between real file and symlink.
facebook/jest#3830

This seems bad... Except that we already *don't* want people to create tests that import individual source files.
All existing cases of us doing so are actually TODOs waiting to be fixed.

So perhaps this requirement isn't too bad because it makes bad code looks bad.

Of course, if we go with this, we'll have to lint against relative requires in tests.
It also makes moving things more painful.
Fix aliased originals getting included in DEV
Shouldn't affect correctness (they were ignored) but fixes DEV size regression.

@gaearon gaearon requested review from bvaughn and trueadm Oct 24, 2017

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 24, 2017

Member

Should be ready for initial review.

Member

gaearon commented Oct 24, 2017

Should be ready for initial review.

@bvaughn

bvaughn approved these changes Oct 24, 2017 edited

This is a pretty large change-set. I skimmed it, paying more attention to the bits in scripts/rollup.

I also did a build-and-sync locally and ran Catalyst with it for a couple of minutes. Had to update ReactNative.js to point to the new haste module name (ReactNativeFiber-* -> ReactNativeRenderer-*) but otherwise it seemed...ok. (I would expect this sort of change to fail fast and hard, if it broke things.)

Left a couple of minor thoughts. Overall this looks like a nice cleanup. 👍

// TODO: direct imports like some-package/src/* are bad. Fix me.
const {
injectInternals,
} = require('react-reconciler/src/ReactFiberDevToolsHook');

This comment has been minimized.

@bvaughn

bvaughn Oct 24, 2017

Contributor

How do you feel about adding a lint rule for this?

Something like...

export default function(context) {
  return {
    Identifier(node) {
      if (node.name === 'require') {
        const path = node.parent.arguments[0].value;
        
        if (path.match(/^react.*\/src/)) {
	      context.report(node, 'Do not import project internals');
        }
      }
    }
  };
};

This would make us less likely to add more of these.

@bvaughn

bvaughn Oct 24, 2017

Contributor

How do you feel about adding a lint rule for this?

Something like...

export default function(context) {
  return {
    Identifier(node) {
      if (node.name === 'require') {
        const path = node.parent.arguments[0].value;
        
        if (path.match(/^react.*\/src/)) {
	      context.report(node, 'Do not import project internals');
        }
      }
    }
  };
};

This would make us less likely to add more of these.

This comment has been minimized.

@gaearon

gaearon Oct 24, 2017

Member

Yea, definitely. On todo list above after we know the change itself works.

@gaearon

gaearon Oct 24, 2017

Member

Yea, definitely. On todo list above after we know the change itself works.

// Module provided by RN:

This comment has been minimized.

@bvaughn

bvaughn Oct 24, 2017

Contributor

👍

@bvaughn

bvaughn Oct 24, 2017

Contributor

👍

@@ -0,0 +1 @@
module.exports = require('ReactCurrentOwner');

This comment has been minimized.

@bvaughn

bvaughn Oct 24, 2017

Contributor

Note to self: I don't understand the purpose of scripts/rollup/shims/rollup/*

Update: Looks like it maps to a combination of forkedFBModules and getShims in modules.js. Still not entirely sure why it's necessary. Seems like it's related some some quirk/limitation of Rollup?

@bvaughn

bvaughn Oct 24, 2017

Contributor

Note to self: I don't understand the purpose of scripts/rollup/shims/rollup/*

Update: Looks like it maps to a combination of forkedFBModules and getShims in modules.js. Still not entirely sure why it's necessary. Seems like it's related some some quirk/limitation of Rollup?

This comment has been minimized.

@gaearon

gaearon Oct 24, 2017

Member

There should be comments close to getShims and its usage explaining.

The object-assign shim replaces object-assign reference in anything that depends on React with assign implementation exposed on React internals. This avoids ReactDOM UMD having to ship its own polyfill of object-assign while React UMD already includes one.

We use the www shims for overriding some modules in React. Basically we want to have some requires be "untouched" and actually require modules from www instead of our implementations in the open source. We can't just tell Rollup to turn require('react/src/ReactCurrentOwner') into require('ReactCurrentOwner') because then it will attempt to resolve it (and not find it). The trick with a shim file and a separate plugin to "transform" the original file to null in memory is the only reliable way I found to completely "rewire" these requires to external modules in Rollup.

Some of these hacks might fall away or become simpler after ES modules. CommonJS is afterthought in Rollup and it breaks some more unusual cases. It might be that we're in that territory, and migrating to ESM will allow to revisit and simplify those hacks.

@gaearon

gaearon Oct 24, 2017

Member

There should be comments close to getShims and its usage explaining.

The object-assign shim replaces object-assign reference in anything that depends on React with assign implementation exposed on React internals. This avoids ReactDOM UMD having to ship its own polyfill of object-assign while React UMD already includes one.

We use the www shims for overriding some modules in React. Basically we want to have some requires be "untouched" and actually require modules from www instead of our implementations in the open source. We can't just tell Rollup to turn require('react/src/ReactCurrentOwner') into require('ReactCurrentOwner') because then it will attempt to resolve it (and not find it). The trick with a shim file and a separate plugin to "transform" the original file to null in memory is the only reliable way I found to completely "rewire" these requires to external modules in Rollup.

Some of these hacks might fall away or become simpler after ES modules. CommonJS is afterthought in Rollup and it breaks some more unusual cases. It might be that we're in that territory, and migrating to ESM will allow to revisit and simplify those hacks.

This comment has been minimized.

@bvaughn

bvaughn Oct 25, 2017

Contributor

Yeah, I saw the comments. I just didn't fully understand them I guess. The third paragraph above helps me understand a little.

@bvaughn

bvaughn Oct 25, 2017

Contributor

Yeah, I saw the comments. I just didn't fully understand them I guess. The third paragraph above helps me understand a little.

Show outdated Hide outdated scripts/rollup/bundles.js Outdated
entry: 'react-art',
global: 'ReactART',
externals: ['react'],
babel: opts =>

This comment has been minimized.

@bvaughn

bvaughn Oct 24, 2017

Contributor

nit: This could have a more meaningful name? Maybe like babelOptionsHook? I dunno. Names are hard.

@bvaughn

bvaughn Oct 24, 2017

Contributor

nit: This could have a more meaningful name? Maybe like babelOptionsHook? I dunno. Names are hard.

This comment has been minimized.

@gaearon

gaearon Oct 24, 2017

Member

I'll probably do another pass renaming them in this PR. I oscillated between long and short names here and used short ones. But maybe I was just tired of complex config and tried to simplify the perception by using shorter words. I don't care strongly either way and will think about them again.

@gaearon

gaearon Oct 24, 2017

Member

I'll probably do another pass renaming them in this PR. I oscillated between long and short names here and used short ones. But maybe I was just tired of complex config and tried to simplify the perception by using shorter words. I don't care strongly either way and will think about them again.

Show outdated Hide outdated scripts/rollup/build.js Outdated
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 24, 2017

Member

Moved other follow up items into #11275.

This is getting pretty painful to keep up to date now so I'd prefer landing and addressing follow ups later this week.

Member

gaearon commented Oct 24, 2017

Moved other follow up items into #11275.

This is getting pretty painful to keep up to date now so I'd prefer landing and addressing follow ups later this week.

@gaearon gaearon referenced this pull request Oct 24, 2017

Closed

Yarn Workspaces Follow-up #11275

13 of 19 tasks complete

@gaearon gaearon merged commit 1eed302 into facebook:master Oct 24, 2017

1 check was pending

ci/circleci Your tests are queued behind your running builds
Details
@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Oct 25, 2017

Contributor

That's fine of course 👍 I'm always happy to iterate on stuff like that.

Contributor

bvaughn commented Oct 25, 2017

That's fine of course 👍 I'm always happy to iterate on stuff like that.

@gaearon gaearon referenced this pull request Oct 25, 2017

Closed

Get rid of providesModule #6336

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Drop Haste (facebook#11303)
* Use relative paths in packages/react

* Use relative paths in packages/react-art

* Use relative paths in packages/react-cs

* Use relative paths in other packages

* Fix as many issues as I can

This uncovered an interesting problem where ./b from package/src/a would resolve to a different instantiation of package/src/b in Jest.

Either this is a showstopper or we can solve it by completely fobbidding remaining /src/.

* Fix all tests

It seems we can't use relative requires in tests anymore. Otherwise Jest becomes confused between real file and symlink.
facebook/jest#3830

This seems bad... Except that we already *don't* want people to create tests that import individual source files.
All existing cases of us doing so are actually TODOs waiting to be fixed.

So perhaps this requirement isn't too bad because it makes bad code looks bad.

Of course, if we go with this, we'll have to lint against relative requires in tests.
It also makes moving things more painful.

* Prettier

* Remove @providesModule

* Fix remaining Haste imports I missed earlier

* Fix up paths to reflect new flat structure

* Fix Flow

* Fix CJS and UMD builds

* Fix FB bundles

* Fix RN bundles

* Prettier

* Fix lint

* Fix warning printing and error codes

* Fix buggy return

* Fix lint and Flow

* Use Yarn on CI

* Unbreak Jest

* Fix lint

* Fix aliased originals getting included in DEV

Shouldn't affect correctness (they were ignored) but fixes DEV size regression.

* Record sizes

* Fix weird version in package.json

* Tweak bundle labels

* Get rid of output option by introducing react-dom/server.node

* Reconciler should depend on prop-types

* Update sizes last time

@gaearon gaearon deleted the gaearon:no-haste-2 branch Dec 14, 2017

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