Skip to content

Add Scripts helper classes #109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

camillobruni
Copy link
Contributor

  • Add ShellScripts and BrowserScripts helper classes
  • Use temp array for scripts in both cases
  • Directly add promise error handler for browser scripts
  • Move runCode to the corresponding scripts class

Copy link

netlify bot commented Jul 31, 2025

Deploy Preview for webkit-jetstream-preview ready!

Name Link
🔨 Latest commit dcc0388
🔍 Latest deploy log https://app.netlify.com/projects/webkit-jetstream-preview/deploys/6890e0a20183d20008d3744b
😎 Deploy Preview https://deploy-preview-109--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 camillobruni requested review from danleh and kmiller68 July 31, 2025 12:24
@camillobruni camillobruni requested a review from danleh July 31, 2025 14:21
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 nits.

throw new Error("addWithURL not supported");
}

addUtils() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if we anyways do this always, can't this be put into the ctor and remove this method?

this.scripts = [];
}

run() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have a short comment what this is doing? IIUC, this is basically a builder pattern, and run doesn't actually run any scripts, but finishes assembling in a way that it can be later run by the driver.

So maybe also rename to assembleScripts and let the comment mention that this object is not supposed to be used afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does actually run the scripts. The moment we attach them to the DOM or call loadString we do evaluate the script.

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