Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Reset SourcesTree when necessary#5200

Merged
jasonLaster merged 2 commits into
firefox-devtools:masterfrom
MikeRatcliffe:fix-duplicate-source-tree-nodes
Jan 28, 2018
Merged

Reset SourcesTree when necessary#5200
jasonLaster merged 2 commits into
firefox-devtools:masterfrom
MikeRatcliffe:fix-duplicate-source-tree-nodes

Conversation

@MikeRatcliffe
Copy link
Copy Markdown

@MikeRatcliffe MikeRatcliffe commented Jan 25, 2018

Pull request #5004 introduced an issue where the SourcesTree was not updated when navigating to a new URL. In reality it was setting the tree using props.blah instead of nextProps.blah.

This also fixes a couple of undefined errors I was randomly getting in the console.

wldcordeiro
wldcordeiro approved these changes Jan 25, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@wldcordeiro wldcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I've made the mistake of using the old props too 😆 Odd that the yarn.lock file changed though.

@jasonLaster
Copy link
Copy Markdown
Contributor

jasonLaster commented Jan 25, 2018

Oh wow - that's a nice catch. Would it be possible to add a new unit test for SourcesTree?

Here is a unit test for the Outline component which is similar
https://github.com/devtools-html/debugger.html/blob/master/src/components/tests/Outline.spec.js#L1

@lukaszsobek
Copy link
Copy Markdown
Contributor

lukaszsobek commented Jan 25, 2018 •
edited
Loading

Nice catch! A test would be great.

lukaszsobek
lukaszsobek reviewed Jan 26, 2018
View reviewed changes
Comment thread src/components/PrimaryPanes/SourcesTree.js Outdated
item.contents.get &&
item.contents.has("id") &&
sources.has(item.contents.get("id")) &&
sources.get(item.contents.get("id")).get("isBlackBoxed")
Copy link
Copy Markdown
Contributor

@lukaszsobek lukaszsobek Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this might be split into two parts to reduce the time it takes to parse.

Copy link
Copy Markdown
Author

@MikeRatcliffe MikeRatcliffe Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

lukaszsobek
lukaszsobek reviewed Jan 26, 2018
View reviewed changes
Comment thread src/components/PrimaryPanes/SourcesTree.js Outdated
@@ -212,7 +214,7 @@ class SourcesTree extends Component<Props, State> {

if (!nodeHasChildren(item)) {
const source = sources.get(item.contents.get("id"));
Copy link
Copy Markdown
Contributor

@lukaszsobek lukaszsobek Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could be able to set this a bit further above and reuse here.

Copy link
Copy Markdown
Author

@MikeRatcliffe MikeRatcliffe Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@MikeRatcliffe MikeRatcliffe force-pushed the fix-duplicate-source-tree-nodes branch from 156605c to 1183310 Compare January 26, 2018 09:44
@MikeRatcliffe
Copy link
Copy Markdown
Author

MikeRatcliffe commented Jan 26, 2018 •
edited
Loading

I have created a test... it seemed way too easy so if I have done anything dumb then please let me know.

@MikeRatcliffe MikeRatcliffe force-pushed the fix-duplicate-source-tree-nodes branch from 1183310 to 4038c4a Compare January 26, 2018 13:10
@jasonLaster
Copy link
Copy Markdown
Contributor

jasonLaster commented Jan 26, 2018

The tests are a good start. I think we want to have a test that simulates changing the sources via setting new props:

this test has a good example of that https://github.com/devtools-html/debugger.html/blob/master/src/components/Editor/tests/Editor.spec.js#L68-L73

@MikeRatcliffe MikeRatcliffe force-pushed the fix-duplicate-source-tree-nodes branch from 4038c4a to d6bb3b4 Compare January 26, 2018 16:44
@MikeRatcliffe MikeRatcliffe changed the title Reset SourcesTree when necessary [WIP] Reset SourcesTree when necessary Jan 26, 2018
@MikeRatcliffe MikeRatcliffe force-pushed the fix-duplicate-source-tree-nodes branch from d6bb3b4 to b61a318 Compare January 26, 2018 16:52
@MikeRatcliffe MikeRatcliffe changed the title [WIP] Reset SourcesTree when necessary Reset SourcesTree when necessary Jan 26, 2018
@MikeRatcliffe
Copy link
Copy Markdown
Author

MikeRatcliffe commented Jan 26, 2018

Should be good to go now.

@jasonLaster jasonLaster force-pushed the fix-duplicate-source-tree-nodes branch from b61a318 to 0595a24 Compare January 27, 2018 23:39
@jasonLaster
Copy link
Copy Markdown
Contributor

jasonLaster commented Jan 27, 2018

just rebased.

@jasonLaster jasonLaster force-pushed the fix-duplicate-source-tree-nodes branch from 0595a24 to a7363d1 Compare January 28, 2018 01:34
@jasonLaster jasonLaster merged commit 8fe140c into firefox-devtools:master Jan 28, 2018
@MikeRatcliffe MikeRatcliffe deleted the fix-duplicate-source-tree-nodes branch January 29, 2018 09:29
jasonLaster pushed a commit that referenced this pull request Jan 29, 2018
@jasonLaster
Reset SourcesTree when necessary (#5200)
ffeeacd 
* Refresh SourcesTree when necessary

* Revert "Revert "Refactors displaying source tree updates (#5004)""

This reverts commit 8800af8.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@lukaszsobek lukaszsobek lukaszsobek left review comments

@wldcordeiro wldcordeiro wldcordeiro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MikeRatcliffe @jasonLaster @lukaszsobek @wldcordeiro