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

[Flow] Upgrade to 70 - #6500

Merged
jasonLaster merged 3 commits into
firefox-devtools:masterfrom
jasonLaster:flow-bump
Jun 18, 2018
Merged

[Flow] Upgrade to 70#6500
jasonLaster merged 3 commits into
firefox-devtools:masterfrom
jasonLaster:flow-bump

Conversation

@jasonLaster

@jasonLaster jasonLaster commented Jun 12, 2018

Copy link
Copy Markdown
Contributor

Fixes Issue: #5687

Summary of Changes

Comment thread src/components/QuickOpenModal.js Outdated
): string | React$Element<"div"> {
if (typeof candidateString != "string") {
return "";
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

maybe we should only call this if the candidate is a string

Comment thread src/components/QuickOpenModal.js Outdated
...result,
title: this.renderHighlight(result.title, basename(newQuery), "title")
};
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i moved this to its own function to simplify the typing but i think we can move it back...

selectedFrame: Frame
) {
if (e.nativeEvent.which == 3) {
if (e.which == 3) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i couldnt find nativeEvent in the types... lets see if this still works

Comment thread src/utils/quick-open.js
id: string,
value: string,
title: string,
title: string | React$Element<"div">,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

when we use fuzzaldrin to fuzzy highlight we return a React Element

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Comment thread src/workers/parser/getSymbols.js Outdated
symbols.classes.push({
name: path.node.id.name,
parent: path.node.superClass,
parent: { name: path.node.superClass },

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we had a bug in inComponent that needed this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, this seems to break preview however as the snapshots are not generating properly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm going to revert this change, if there's a bug we should open an issue for it. 😄

screen shot 2018-06-18 at 9 19 45 am

hasJsx: false,
hasTypes: false
hasTypes: false,
loading: false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can no longer check if a property exists w/ flow... with that said, we might want to try another option here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 interesting note, just saw that in 0.68 release notes.

We now check if we're dealing with a string before highlighting so we aren't producing empty
innerHTML.
This broke some tests but was cited as being for a bug fix. It'd be preferable if that's done in a
PR of its own with the appropriate changes so it doesn't sneak breakage elsewhere.

@wldcordeiro wldcordeiro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some notes and got the test suite passing. This looks ready to go @jasonLaster

@jasonLaster
jasonLaster merged commit 2eccc73 into firefox-devtools:master Jun 18, 2018
jasonLaster added a commit that referenced this pull request Jun 28, 2018
jasonLaster added a commit that referenced this pull request Jun 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants