initial work on adding hooks into single-spa for devtools functionality #266
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheMcMurder
reviewed
Jan 29, 2019
src/devtools/devtools.helper.js
Outdated
| } | ||
| return app | ||
| }) | ||
| } |
There was a problem hiding this comment.
I might be wrong here, IIRC because app is an object it's passed by reference. Meaning this delete methods are going to modify the actual app instead of modifying a duplicate of the app
src/devtools/devtools.helper.js
Outdated
| @@ -0,0 +1,22 @@ | |||
| export function removeCyclicReferences(apps) { | |||
| // we must get rid of cyclic references before we can return apps -- otherwise, devtools gets an error because JSON.stringify doesn't resolve cyclic references | |||
There was a problem hiding this comment.
Does devTools need to stringify the apps? React Components are also circular, what are they doing differently (or are they doing the same thing)
I'm not sure I fully understand why this is necessary
added 2 commits
January 29, 2019 12:41
…an just remove the cyclical referernces altogether, and rely on the closures that already exist.
TheMcMurder
reviewed
Jan 29, 2019
| @@ -15,6 +15,11 @@ export function getAppNames() { | |||
| return apps.map(toName); | |||
| } | |||
|
|
|||
| // used in devtools, not (currently) exposed as a single-spa API | |||
| export function getRawAppData() { | |||
| return [...apps]; | |||
There was a problem hiding this comment.
I still have some worry about exposing the full internal representation to the dev tools, because we can't call the methods on them anyways, I'm still interested in these values being a copy of the internal representation long term.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Devtools need raw app info, so I had to add that function
Since the data being passed between devtools and the inspected window is JSON.stringify-ed, cyclic references break that process. So to clean up the apps array for that, we have to remove those cyclic references.