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.

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

feat: Add configuration option to enable javascript #341

Merged
merged 1 commit into from
Sep 3, 2019

Conversation

wwilsman
Copy link
Contributor

@wwilsman wwilsman commented Sep 3, 2019

Purpose

Enabling JavaScript must be done on a per-snapshot basis and in rare cases where the user needs to enable it for every snapshot, it becomes tedious to add it to every snapshot call.

Approach

Add an enable-javascript option to the configuration file to enable setting this option for all snapshots in a single setting.

I know we don't recommend setting this option normally, however if needed it can be a pain to set this for every single snapshot.

@wwilsman wwilsman requested a review from Robdel12 September 3, 2019 18:24
enableJavaScript: request.body.enableJavaScript,
enableJavaScript: request.body.enableJavaScript != null
? request.body.enableJavaScript
: configuration.snapshot['enable-javascript'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nullish operator would've been perfect here:

request.body.enableJavaScript ?? configuration.snapshot['enable-javascript']

Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁 I'm all for the change. This will be helpful for folks we don't support very well yet (looking at you shadow DOM 👀 )

edit: don't forget to add feat: to your PR title (and since this is a single commit, make sure that carries into the merge commit)

@Robdel12
Copy link
Contributor

Robdel12 commented Sep 3, 2019

Oh, also don't forget to update the SDK config docs

@wwilsman wwilsman changed the title Add configuration option to enable javascript feat: Add configuration option to enable javascript Sep 3, 2019
@wwilsman
Copy link
Contributor Author

wwilsman commented Sep 3, 2019

I don't think the docs need an update since this is a "hidden" option. We don't document enableJavaScript anywhere else, do we? If we wanted to, I'd say we should update all docs across the board

@wwilsman wwilsman merged commit fb05050 into master Sep 3, 2019
@wwilsman wwilsman deleted the ww/enable-js-yml branch September 3, 2019 19:50
djones pushed a commit that referenced this pull request Sep 3, 2019
# [0.11.0](v0.10.1...v0.11.0) (2019-09-03)

### Features

* Add configuration option to enable javascript ([#341](#341)) ([fb05050](fb05050))
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