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

[DevTools] [Context] Legacy Context #16617

Merged

Conversation

@hristo-kanchev
Copy link
Contributor

commented Aug 30, 2019

Fixes: #16466 and #16679

Description:
We are now changing the context label in DevTools based on if the component is using the legacy way of adding context.

@hristo-kanchev hristo-kanchev changed the title [DevTools] Legacy Context [DevTools] [Context] Legacy Context Aug 30, 2019
@sizebot

This comment has been minimized.

Copy link

commented Aug 30, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against de31745

@gaearon

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Is it possible to add some sort of a test?

@hristo-kanchev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Is it possible to add some sort of a test?

Tests

@hristo-kanchev

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

@gaearon Did you manage to check the tests?

Copy link
Contributor

left a comment

Thanks! This looks nice. :)

@@ -2128,7 +2130,13 @@ export function attach(
) {
canViewSource = true;
if (stateNode && stateNode.context != null) {
context = stateNode.context;
const shouldHideContext =

This comment has been minimized.

Copy link
@bvaughn

bvaughn Sep 10, 2019

Contributor

This could probably use an explanatory inline comment. (I'll add one.)

@@ -11,10 +11,13 @@ import typeof ReactTestRenderer from 'react-test-renderer';
import type {GetInspectedElementPath} from 'react-devtools-shared/src/devtools/views/Components/InspectedElementContext';
import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
import type Store from 'react-devtools-shared/src/devtools/store';
import React, {Component, createContext} from 'react';
import {Fragment} from '../../../../../../../../../../Applications/WebStorm.app/Contents/plugins/JavaScriptLanguage/jsLanguageServicesImpl/flow/react';

This comment has been minimized.

Copy link
@bvaughn

bvaughn Sep 10, 2019

Contributor

😆

This comment has been minimized.

Copy link
@hristo-kanchev

hristo-kanchev Sep 10, 2019

Author Contributor

Freakin' WebStorm 😆 #NeverTrustYourEditor

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

@gaearon Did you manage to check the tests?

What's this comment about? Is this PR safe to merge or do we need to check on something first?

@gaearon

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

I think this was meant as “@gaearon can you review again now that tests were added”. Lgtm

@@ -753,6 +753,9 @@ export function attach(
// Can view component source location.
canViewSource: type === ElementTypeClass || type === ElementTypeFunction,

// Only legacy context exists in legacy versions.
hasLegacyContext: true,

This comment has been minimized.

Copy link
@gaearon

gaearon Sep 10, 2019

Member

Should we also hide empty one though in the legacy mode?

This comment has been minimized.

Copy link
@bvaughn

bvaughn Sep 10, 2019

Contributor

This value isn't used to hide the context label in the front end.. It just specifies whether the context is modern vs legacy. I guess it's a little weird to have a boolean value for this. Maybe we should use an enum? context type: none, modern, legacy.

Anyway, an empty legacy context won't be shown on the frontend because the InspectedElementTree component hides itself if the value it's passed is null or an empty object.

@bvaughn bvaughn merged commit 4ef6387 into facebook:master Sep 10, 2019
13 checks passed
13 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.