Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add support for injectScript #151

Merged
merged 3 commits into from Jan 18, 2022
Merged

feat: add support for injectScript #151

merged 3 commits into from Jan 18, 2022

Conversation

mbrevda
Copy link
Contributor

@mbrevda mbrevda commented Jan 4, 2022 •
edited

Noticed the option was missing, would be great to get this added!

@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 13, 2022

Hey - any way this can be prioritized?

tkadlec
tkadlec suggested changes Jan 13, 2022
View reviewed changes
Copy link
Contributor

@tkadlec tkadlec left a comment

Choose a reason for hiding this comment

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

Two quick changes and we're all set here. Thanks for hooking this up @mbrevda!

README.md Outdated Show resolved Hide resolved
lib/mapping.js Outdated Show resolved Hide resolved
tkadlec
tkadlec reviewed Jan 13, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 14, 2022

sure, here you go!

tkadlec
tkadlec approved these changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@tkadlec tkadlec left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @mbrevda!

@tkadlec
Copy link
Contributor

tkadlec commented Jan 14, 2022

Whoops! One more quick change, @mbrevda to fix the tests and then we can merge.

Looks like we need to update the fixture here: https://github.com/WebPageTest/webpagetest-api/blob/5889c0d3ecdad071bfdde59b3c525548fe1acf38/test/fixtures/command-line/help-test.txt#L35

So that it uses <injectScript> instead of <script>

@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 15, 2022

like that?

@tkadlec
Copy link
Contributor

tkadlec commented Jan 18, 2022

Perfect! Thanks @mbrevda!

@tkadlec tkadlec merged commit fa53d34 into catchpoint:master Jan 18, 2022
@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 18, 2022

Pleasure - eta on version bump and github action update to use this?

@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 25, 2022

Any ETA on when this will be published?

@mbrevda
Copy link
Contributor Author

mbrevda commented Feb 16, 2022

ping - can we get this published please?

@tkadlec
Copy link
Contributor

tkadlec commented Feb 16, 2022

Done! Sorry it took a bit. We'll have to roll the other updates out in another release, but this way we get your updates out the door. Thanks for doing this!

@mbrevda
Copy link
Contributor Author

mbrevda commented Feb 17, 2022

Awesome, thanks! Can we please bump the github action to use it? catchpoint/WebPageTest.github-action#9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkadlec tkadlec tkadlec approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mbrevda @tkadlec