Update to working open, switch to ESM#499
Conversation
|
Hey, unless I'm misunderstanding something important, updating to a version of I'd prefer to keep this change as minimally scoped as possible. Also, it would be helpful to understand the nature of the existing version of |
|
It doesn’t support Plasma 6, so it’s completely useless and does nothing. Only version 10 is fixed, since it ships |
|
OK, done. That requires renaming the CLI file, but of course that has much less impact than making everything ESM |
| urlToOpen = `file://${htmlPath}` | ||
| urlToOpen = `file://${htmlPath}` | ||
| } | ||
| } |
There was a problem hiding this comment.
I've only actually tested this on OS X, so it's possible this issue exists for other platforms too.
To minimize the possibility of breakage, can we maintain this without the process.platform check?
If you'd like to avoid this step on the specific platform you're using, I'm also open to skipping this step on any platforms you can test.
There was a problem hiding this comment.
The open package has 3 platform branches: 'win32', 'darwin', and else. This means it tries to use xdg-open on all platforms that aren’t windows or macOS.
I tried on Windows, and indeed, the hash was also stripped, so now it only skips the workaround for platforms using xdg-open (Linux and I guess BSDs and so on)
|
Thanks for minimizing the surface area of this change! One more small tweak to that effect then this should be good to land |
|
OK, done! You were right about your platform concerns, I adapted my change. |
|
@flying-sheep Thanks! This is published on npm as part of speedscope@v1.22.1 and later |
See #498
I regenerated all the autogenerated files, but there was an issue with
demangle.wasm.js: It also generated themodule.exports = Module; module.exports.default = Modulelines for some reason, so I manually deleted them.If you know how to make it only emit ESM exports and leave out the CommonJS exports, I can add that to the makefile.