Skip to content

Import preact devtools renderer code#2148

Merged
marvinhagemeister merged 10 commits into
masterfrom
preact-devtools
Dec 9, 2019
Merged

Import preact devtools renderer code#2148
marvinhagemeister merged 10 commits into
masterfrom
preact-devtools

Conversation

@marvinhagemeister

@marvinhagemeister marvinhagemeister commented Nov 24, 2019

Copy link
Copy Markdown
Member

This is an integration for our custom devtools 🎉

Screenshot from 2019-11-24 22-34-26

Tasks:

  • Convert tests written in TS to JS
  • Fix infinite loop in jsonify with some Suspense components in demo app
  • Fix createContext value always parsed as an object
  • Code Coverage 😬

@coveralls

coveralls commented Nov 24, 2019

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 95e8259 on preact-devtools into b40d02c on master.

@marvinhagemeister marvinhagemeister force-pushed the preact-devtools branch 2 times, most recently from 0cd3c5c to bc7d3af Compare November 29, 2019 08:07
@marvinhagemeister marvinhagemeister marked this pull request as ready for review November 30, 2019 10:26
@marvinhagemeister

Copy link
Copy Markdown
Member Author

I'm unsure if I should move it to preact/devtools with this PR or with a separate one. Leaving it in preact/debug for now would have the advantage of keeping the existing imports the same for users.

@marvinhagemeister marvinhagemeister force-pushed the preact-devtools branch 2 times, most recently from 485b9c3 to 9dcfd64 Compare December 5, 2019 09:42
@marvinhagemeister

Copy link
Copy Markdown
Member Author

I think this PR is ready to review since a few days. If there is anything to make reviewing it easier, please reach out.

@JoviDeCroock JoviDeCroock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MVP Marvin!

Comment thread debug/src/devtools/10/IdMapper.js Outdated
// from the start. `performance.now()` will always return a double.
// See https://github.com/facebook/react/issues/14365
// and https://slidr.io/bmeurer/javascript-engine-fundamentals-the-good-the-bad-and-the-ugly
vnode.startTime = NaN;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Today I learned, thanks!

* @returns {boolean}
*/
export function isRoot(vnode) {
return vnode._parent == null && vnode.type === Fragment;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes me wonder if we need a special indicator for this node

@marvinhagemeister marvinhagemeister Dec 6, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, true, we may need something to mark root nodes as roots instead of plain Fragments in the future. Right now this is an ok workaround and if it's just for the devtools it's fine by me. But if it makes it some future feature easier in core I'm all for it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants