enable dom_storage for node [devtools/devtools-frontend : main]
ryuhei shima (Gerrit)
ryuhei shima added 1 comment![]( "Open in Gerrit")
Capability.JS | Capability.NETWORK | Capability.TARGET | Capability.IO | Capability.DOM_STORAGE;In Node.js, we want to expose only DOM Storage rather than the entire DOM, so I'm adding a new DOM_STORAGE capability.
Related details
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd added 1 comment![]( "Open in Gerrit")
What happens if we debug an older version of Node.js where DOM storage is not available, and then navigate to the application panel? Will DevTools keep working?
DevTools in general does not have to deal with backwards compatibility as the corresponding Chrome version is always fixed. This means many code paths don't assume that CDP calls can fail.
Related details
- Jack Franklin
- Philip Pfaffe
- ryuhei shima
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ryuhei shima (Gerrit)
ryuhei shima added 1 comment![]( "Open in Gerrit")
What happens if we debug an older version of Node.js where DOM storage is not available, and then navigate to the application panel? Will DevTools keep working?
DevTools in general does not have to deal with backwards compatibility as the corresponding Chrome version is always fixed. This means many code paths don't assume that CDP calls can fail.
Yes, DevTools keeps working.
When DOM Storage is not available, "LocalStorage" tab just displays “No local storage detected”.
Related details
- Jack Franklin
- Philip Pfaffe
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd added 5 comments![]( "Open in Gerrit")
ryuhei shimaWhat happens if we debug an older version of Node.js where DOM storage is not available, and then navigate to the application panel? Will DevTools keep working?
DevTools in general does not have to deal with backwards compatibility as the corresponding Chrome version is always fixed. This means many code paths don't assume that CDP calls can fail.
Yes, DevTools keeps working.
When DOM Storage is not available, "LocalStorage" tab just displays “No local storage detected”.
Acknowledged
Mostly lgtm, some minor point to fix build correctness.
Capability.JS | Capability.NETWORK | Capability.TARGET | Capability.IO | Capability.DOM_STORAGE;In Node.js, we want to expose only DOM Storage rather than the entire DOM, so I'm adding a new DOM_STORAGE capability.
Acknowledged
const Resources = await import('../../panels/application/application.js');Lets follow the existing pattern and store the imported module in a `loadedApplicationModule` variable. This requires us to declare the import (for the type) and also modify the BUILD.gn file to declare the application panel as a dependency. Otherwise, a clean build that builds the `node_app` entrypoint will not actually build and include the application panel.
return Resources.ResourcesPanel.ResourcesPanel.instance({Probably wanna pass `forceNew: true` here just in case.
Related details
- Jack Franklin
- Philip Pfaffe
- ryuhei shima
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ryuhei shima (Gerrit)
ryuhei shima added 3 comments![]( "Open in Gerrit")
origin: origin === 'file://' ? components[0] as Platform.DevToolsPath.UrlString : origin,Sorry, I’ve added an additional change.
Since Node.js wants to use the file URL of localStorage as the StorageKey, when the URL is file://, we’d like to use the original URL itself as the origin.
const Resources = await import('../../panels/application/application.js');Lets follow the existing pattern and store the imported module in a `loadedApplicationModule` variable. This requires us to declare the import (for the type) and also modify the BUILD.gn file to declare the application panel as a dependency. Otherwise, a clean build that builds the `node_app` entrypoint will not actually build and include the application panel.
Fixed.
return Resources.ResourcesPanel.ResourcesPanel.instance({Probably wanna pass `forceNew: true` here just in case.
- Jack Franklin
- Philip Pfaffe
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd added 3 comments![]( "Open in Gerrit")
origin: origin === 'file://' ? components[0] as Platform.DevToolsPath.UrlString : origin,Sorry, I’ve added an additional change.
Since Node.js wants to use the file URL of localStorage as the StorageKey, when the URL is file://, we’d like to use the original URL itself as the origin.
Can you add that as a comment in the code as well? Otherwise the impl differs from blink for no apparent reason.
const Resources = await import('../../panels/application/application.js');ryuhei shimaLets follow the existing pattern and store the imported module in a `loadedApplicationModule` variable. This requires us to declare the import (for the type) and also modify the BUILD.gn file to declare the application panel as a dependency. Otherwise, a clean build that builds the `node_app` entrypoint will not actually build and include the application panel.
Fixed.
Please add an entry to the `deps` list of the `meta` devtools_entrypoint in the BUILD.gn file in the same directory.
return Resources.ResourcesPanel.ResourcesPanel.instance({ryuhei shimaProbably wanna pass `forceNew: true` here just in case.
Fixed.
- Jack Franklin
- Philip Pfaffe
- ryuhei shima
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shawon Mollah (Gerrit)
Shawon Mollah added 1 comment![]( "Open in Gerrit")
origin: origin === 'file://' ? components[0] as Platform.DevToolsPath.UrlString : origin,Simon ZündSorry, I’ve added an additional change.
Since Node.js wants to use the file URL of localStorage as the StorageKey, when the URL is file://, we’d like to use the original URL itself as the origin.
Can you add that as a comment in the code as well? Otherwise the impl differs from blink for no apparent reason.
- Jack Franklin
- Philip Pfaffe
- Simon Zünd
- ryuhei shima
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ryuhei shima (Gerrit)
ryuhei shima added 1 comment![]( "Open in Gerrit")
origin: origin === 'file://' ? components[0] as Platform.DevToolsPath.UrlString : origin,Simon ZündSorry, I’ve added an additional change.
Since Node.js wants to use the file URL of localStorage as the StorageKey, when the URL is file://, we’d like to use the original URL itself as the origin.
Shawon MollahCan you add that as a comment in the code as well? Otherwise the impl differs from blink for no apparent reason.
Done
Sorry, the comment was left in draft and wasn’t sent. It’s done.
Related details
- Jack Franklin
- Philip Pfaffe
- Shawon Mollah
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ryuhei shima (Gerrit)
ryuhei shima added 1 comment![]( "Open in Gerrit")
const Resources = await import('../../panels/application/application.js');ryuhei shimaLets follow the existing pattern and store the imported module in a `loadedApplicationModule` variable. This requires us to declare the import (for the type) and also modify the BUILD.gn file to declare the application panel as a dependency. Otherwise, a clean build that builds the `node_app` entrypoint will not actually build and include the application panel.
Simon ZündFixed.
Please add an entry to the `deps` list of the `meta` devtools_entrypoint in the BUILD.gn file in the same directory.
Sorry for the delay. I’ve made the changes.
Related details
- Jack Franklin
- Philip Pfaffe
- Shawon Mollah
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd voted and added 2 comments![]( "Open in Gerrit")
Votes added by Simon Zünd
| Code-Review | +1 |
2 comments
"../../panels/application:meta",Apologies, it should be `application:bundle` instead of `application:meta`. I mixed it up. Otherwise lgtm.
const Resources = await import('../../panels/application/application.js');ryuhei shimaLets follow the existing pattern and store the imported module in a `loadedApplicationModule` variable. This requires us to declare the import (for the type) and also modify the BUILD.gn file to declare the application panel as a dependency. Otherwise, a clean build that builds the `node_app` entrypoint will not actually build and include the application panel.
Simon ZündFixed.
ryuhei shimaPlease add an entry to the `deps` list of the `meta` devtools_entrypoint in the BUILD.gn file in the same directory.
Sorry for the delay. I’ve made the changes.
- Jack Franklin
- Philip Pfaffe
- Shawon Mollah
- ryuhei shima
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe voted Code-Review+1![]( "Open in Gerrit")
- Jack Franklin
- Shawon Mollah
- ryuhei shima
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ryuhei shima (Gerrit)
ryuhei shima added 1 comment![]( "Open in Gerrit")
"../../panels/application:meta",Apologies, it should be `application:bundle` instead of `application:meta`. I mixed it up. Otherwise lgtm.
- Jack Franklin
- Philip Pfaffe
- Shawon Mollah
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd voted and added 1 comment![]( "Open in Gerrit")
Votes added by Simon Zünd
| Code-Review | +1 |
1 comment
"../../panels/application:meta",ryuhei shimaApologies, it should be `application:bundle` instead of `application:meta`. I mixed it up. Otherwise lgtm.
I’ve fixed it.
- Jack Franklin
- Philip Pfaffe
- Shawon Mollah
- ryuhei shima
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe voted Code-Review+1![]( "Open in Gerrit")
- Jack Franklin
- Shawon Mollah
- ryuhei shima
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Devtools-frontend LUCI CQ (Gerrit)
Devtools-frontend LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
enable dom_storage for node
This is a change request to enable DOM storage inspection in Node.js.
The Node.js changes are implemented in the following PR.
https://github.com/nodejs/node/pull/61139
- M front_end/core/sdk/StorageKeyManager.ts
- M front_end/core/sdk/Target.ts
- M front_end/entrypoints/node_app/BUILD.gn
- M front_end/entrypoints/node_app/app/NodeMain.ts
- M front_end/entrypoints/node_app/node_app.ts
- M front_end/panels/application/ApplicationPanelSidebar.ts
- M front_end/panels/application/DOMStorageModel.ts
- M front_end/panels/application/ResourcesPanel.ts
Code-Review: +1 by Simon Zünd, +1 by Philip Pfaffe
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |