Copy React ART tests and add hacks to fix them #6775

Merged
merged 3 commits into from May 24, 2016

Conversation

Projects
None yet
4 participants
@sophiebits
Member

sophiebits commented May 14, 2016

This helps us make sure we don't break React ART in a minor or patch release. The idea is to not change these files when making minor or patch changes. Copied directly from react-art with requires fixed. (I also picked a different haste name just in case.)

Then I fixed the tests.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 14, 2016

Member

@facebook/react-core This is kinda icky. My native/host rename mucked this up (getNativeNode) but the devtools warns about missing debug IDs too before this change. We can't really change React ART because we have no way to guarantee that people aren't using React ART v15 with React 15.1.

Member

sophiebits commented May 14, 2016

@facebook/react-core This is kinda icky. My native/host rename mucked this up (getNativeNode) but the devtools warns about missing debug IDs too before this change. We can't really change React ART because we have no way to guarantee that people aren't using React ART v15 with React 15.1.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 14, 2016

Member

Well, apparently we haven't published React ART 15.0. But the point stands, I think.

Member

sophiebits commented May 14, 2016

Well, apparently we haven't published React ART 15.0. But the point stands, I think.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 14, 2016

Member

Can we just actually ship ReactART's source inside the react npm package and then we won't have to worry?

Member

sophiebits commented May 14, 2016

Can we just actually ship ReactART's source inside the react npm package and then we won't have to worry?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 15, 2016

@spicyj updated the pull request.

ghost commented May 15, 2016

@spicyj updated the pull request.

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao May 16, 2016

Member

Can we just actually ship ReactART's source inside the react npm package and then we won't have to worry?

Isn't that the opposite of what we want to do with renderers?

Member

zpao commented May 16, 2016

Can we just actually ship ReactART's source inside the react npm package and then we won't have to worry?

Isn't that the opposite of what we want to do with renderers?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 16, 2016

Member

Ehh… React ART is weird because it isn't its own environment so we can't make things completely separate. Unlike React DOM and React Native where it's okay if they have different versions (and we don't want them to share any state or code). Maybe we can try to make React ART more independent (and that might be a good idea anyway) but we're not set up for that right now.

Member

sophiebits commented May 16, 2016

Ehh… React ART is weird because it isn't its own environment so we can't make things completely separate. Unlike React DOM and React Native where it's okay if they have different versions (and we don't want them to share any state or code). Maybe we can try to make React ART more independent (and that might be a good idea anyway) but we're not set up for that right now.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage May 16, 2016

Member

ART doesn't depend on shared state other than owner and current context.

Just copying all the files from React to ART should be sufficient too.

The question is, if we keep going in the opposite direction of decoupling, when will we ever get to a decoupled state? What spin off effects will we get from doing this?

On May 15, 2016, at 10:27 PM, Ben Alpert notifications@github.com wrote:

Ehh… React ART is weird because it isn't its own environment so we can't make things completely separate. Unlike React DOM and React Native where it's okay if they have different versions (and we don't want them to share any state or code). Maybe we can try to make React ART more independent (and that might be a good idea anyway) but we're not set up for that right now.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub

Member

sebmarkbage commented May 16, 2016

ART doesn't depend on shared state other than owner and current context.

Just copying all the files from React to ART should be sufficient too.

The question is, if we keep going in the opposite direction of decoupling, when will we ever get to a decoupled state? What spin off effects will we get from doing this?

On May 15, 2016, at 10:27 PM, Ben Alpert notifications@github.com wrote:

Ehh… React ART is weird because it isn't its own environment so we can't make things completely separate. Unlike React DOM and React Native where it's okay if they have different versions (and we don't want them to share any state or code). Maybe we can try to make React ART more independent (and that might be a good idea anyway) but we're not set up for that right now.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 16, 2016

Member

Okay, we don't have to merge them. Does this PR look good though? I want to be able to test against React ART even if we consider them decoupled.

Member

sophiebits commented May 16, 2016

Okay, we don't have to merge them. Does this PR look good though? I want to be able to test against React ART even if we consider them decoupled.

+ // We renamed this. Allow the old name for compat. :(
+ if (!instance.getHostNode) {
+ instance.getHostNode = instance.getNativeNode;
+ }

This comment has been minimized.

@zpao

zpao May 16, 2016

Member

Since we didn't ship ReactART 15 yet… we could just make it depend on 15.1 and not support 15.0.

@zpao

zpao May 16, 2016

Member

Since we didn't ship ReactART 15 yet… we could just make it depend on 15.1 and not support 15.0.

src/renderers/art/ReactART.js
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @providesModule ReactARTFifteen

This comment has been minimized.

@zpao

zpao May 16, 2016

Member

Should be fine. We can also exclude this dir when syncing to avoid the clash.

@zpao

zpao May 16, 2016

Member

Should be fine. We can also exclude this dir when syncing to avoid the clash.

This comment has been minimized.

@sophiebits

sophiebits May 16, 2016

Member

Yeah, I was more worried about npm clients.

@sophiebits

sophiebits May 16, 2016

Member

Yeah, I was more worried about npm clients.

This comment has been minimized.

@zpao

zpao May 16, 2016

Member

Could always just not copy this whole directory to lib/ (which we probably want to do for the noop renderer from #6690 too)

--- a/gulpfile.js
+++ b/gulpfile.js
@@ -20,6 +20,7 @@ var paths = {
   react: {
     src: [
       'src/**/*.js',
+      '!src/renders/art/**/*.js',
       '!src/**/__benchmarks__/**/*.js',
       '!src/**/__tests__/**/*.js',
       '!src/**/__mocks__/**/*.js',
@zpao

zpao May 16, 2016

Member

Could always just not copy this whole directory to lib/ (which we probably want to do for the noop renderer from #6690 too)

--- a/gulpfile.js
+++ b/gulpfile.js
@@ -20,6 +20,7 @@ var paths = {
   react: {
     src: [
       'src/**/*.js',
+      '!src/renders/art/**/*.js',
       '!src/**/__benchmarks__/**/*.js',
       '!src/**/__tests__/**/*.js',
       '!src/**/__mocks__/**/*.js',

This comment has been minimized.

@sophiebits

sophiebits May 24, 2016

Member

k, done.

@sophiebits

sophiebits May 24, 2016

Member

k, done.

sophiebits added some commits May 14, 2016

Add (failing) React ART tests
This helps us make sure we don't break React ART in a minor or patch release. The idea is to not change these files when making minor or patch changes. Copied directly from react-art with requires fixed. (I also picked a different haste name just in case.)

@sophiebits sophiebits merged commit 531a6b3 into facebook:master May 24, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot May 24, 2016

@spicyj updated the pull request.

@spicyj updated the pull request.

@zpao zpao added this to the 15-next milestone Jun 1, 2016

zpao added a commit to zpao/react that referenced this pull request Jun 8, 2016

Merge pull request #6775 from spicyj/fix-art
Copy React ART tests and add hacks to fix them
(cherry picked from commit 531a6b3)

zpao added a commit that referenced this pull request Jun 14, 2016

Merge pull request #6775 from spicyj/fix-art
Copy React ART tests and add hacks to fix them
(cherry picked from commit 531a6b3)

@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016

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