Fix broken CLI dependency open

[flying-sheep]

That old open version is very broken on my system, this should make sure that it is and stays up-to-date.

Seems like it’s less disruptive to use any open version instead of pinning old ones forever.

[jlfwong]

Hi @flying-sheep! Thanks for updating. I pin intentionally because otherwise software breaks in weird and wonderful ways at surprising cadences. Semver is great in theory, but in practice I've seen all sorts of things break during minor or patch versions

If you update this PR to pin to a newer version instead, I'm happy to accept that.

[flying-sheep]

Oh, but this PR so far didn‘t even go for semver. I used >= instead. I did that because not upgrading to open 10.x broke it on my system.

This bug is an ideal example of there being no right answer:

• had you trusted that the minimal API we use stays the same even across major releases, everything would work without change (as long as people ignored the lockfile when installing)
• but of course you can’t rely on that, that’s what semver is for: kinda reliable, but if a package does major releases (like open here), one gains little without having e.g. dependabot regularly bumping us to the newest major version
• full pinning without dependabot is the worst of both worlds: people don’t even get bug fixes

So I think using the semver and setting up dependabot to bump open whenever a new release comes out would probably be the best compromise here.

[jlfwong]

Respectfully, this is not how I choose to maintain my project. For other projects and other maintainers, the workflows you describe may be ideal.

For my own personal preferences in terms of dealing with things changing without me making any code changes, and in terms of avoiding dealing with the noisiness of things from dependabot, I'd prefer to continue pinning dependencies.

I hear you that, in this particular case, pinning caused this tool to break for you. But in the 7 years this project has been running, this is the first time that pinning has caused a breakage.

So, as I originally said, I'd be happy to merge this change with an exact version pin

[flying-sheep]

OK, done

[jlfwong]

Thank you!

[jlfwong]

@flying-sheep Is this change working for you?

After merging your change, I tested it as follows:

scripts/prepare-test-installation.sh

This runs through a process similar to a full npm publish & install procedure, but without actually publishing to npm.

Then I switched into the directory it created, and ran bin/cli.js, which does the equivalent of the speedscope script when you run npm install -g speedscope. Upon running that, I see this:

Opening file:///private/var/folders/80/29_gz6bj01nb95w7vf2ty_rw0000gn/T/speedscope-test-installation.b4KzwiDTGd/package/dist/release/index.html in your default browser
(node:48359) ExperimentalWarning: CommonJS module /private/var/folders/80/29_gz6bj01nb95w7vf2ty_rw0000gn/T/speedscope-test-installation.b4KzwiDTGd/package/bin/cli.js is loading ES Module /private/var/folders/80/29_gz6bj01nb95w7vf2ty_rw0000gn/T/speedscope-test-installation.b4KzwiDTGd/package/node_modules/open/index.js using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
TypeError: open is not a function
    at main (/private/var/folders/80/29_gz6bj01nb95w7vf2ty_rw0000gn/T/speedscope-test-installation.b4KzwiDTGd/package/bin/cli.js:106:9)
    at Object.<anonymous> (/private/var/folders/80/29_gz6bj01nb95w7vf2ty_rw0000gn/T/speedscope-test-installation.b4KzwiDTGd/package/bin/cli.js:109:1)
    at Module._compile (node:internal/modules/cjs/loader:1566:14)
    at Object..js (node:internal/modules/cjs/loader:1718:10)
    at Module.load (node:internal/modules/cjs/loader:1305:32)
    at Function._load (node:internal/modules/cjs/loader:1119:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:220:24)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:151:5)
    at node:internal/main/run_main_module:33:47

Usage: speedscope [filepath]

If invoked with no arguments, will open a local copy of speedscope in your default browser.
Once open, you can browse for a profile to import.

If - is used as the filepath, will read from stdin instead.

  cat /path/to/profile | speedscope -

This makes sense to me. When I look at the documentation for open, it says the following:

Warning: This package is native ESM and no longer provides a CommonJS export. If your project uses CommonJS, you will have to convert to ESM or use the dynamic import() function. Please don't open issues for questions regarding CommonJS / ESM.

And indeed our bin/cli.js script uses require rather than import since it was written quite a long time ago.

So that suggests to me that this change requires additional changes, not just bumping a dependency.

I'm going to revert the change for now.

If you're interested in updating this PR to resolve this problem, let me know.

[flying-sheep]

I relied on you having CI for your CLI …

Could you add that so I can try again?