Skip to content

Conversation

camillobruni
Copy link
Contributor

@camillobruni camillobruni commented Sep 1, 2025

For displaying a warning about non-default params we need a way to keep track of what the default values are.
We can reuse the existing code from Speedometer which does exactly that.

  • Copy params.js from Speedometer
  • Implement more uniform params parsing for browser (URLSearchParams) and cli (Map from parsed arguments)
  • Acces params/settings via global JetStreamParams
  • Expose both JetStreamParams and DefaultJetStreamParams making it east to see if we're running with non-default params

Copy link

netlify bot commented Sep 1, 2025

Deploy Preview for webkit-jetstream-preview ready!

Name Link
🔨 Latest commit aac7638
🔍 Latest deploy log https://app.netlify.com/projects/webkit-jetstream-preview/deploys/68b811c9f52fc90008db7b94
😎 Deploy Preview https://deploy-preview-171--webkit-jetstream-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@camillobruni
Copy link
Contributor Author

side-note: once we have this, I can also copy over the dev-menu from speedometer, which makes these parameters a bit more accessible.

Copy link
Contributor

@danleh danleh left a comment

Choose a reason for hiding this comment

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

Overall LGTM, some nits.
The high-level motivation (besides adding a UI menu in the browser later) seems to be (i) refactoring/cleanup and (ii) allowing detection of non-standard parameters, such that we can warn somehow when displaying results.

console.log(" ", tagName);
console.log("");

if (typeof benchmarksByTag !== "undefined") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this check, isn't benchmarksByTag always defined in

const benchmarksByTag = new Map();

In that case, can this just be an assert?

@camillobruni camillobruni requested a review from danleh September 2, 2025 22:01
Copy link
Contributor

@danleh danleh left a comment

Choose a reason for hiding this comment

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

LGTM with question.

constructor(sourceParams = undefined) {
if (sourceParams)
this._copyFromSearchParams(sourceParams);
if (!this.developerMode)
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed developer mode from the CLI options above, but not here. Is this intended? I'm still not sure I got the intuition behind the developer mode, how is it different / what does it ensure additionally on top of just warning when folks run JetStream with non-default parameters?

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.

2 participants