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

fix(ses): reject unsupported lockdownOptions mathTaming + dateTaming #2583

Closed
wants to merge 1 commit into from

Conversation

kumavis
Copy link
Member

@kumavis kumavis commented Oct 11, 2024

This is a breaking change.

support for lockdown options mathTaming and dateTaming were removed so long ago I'm having trouble finding a proper reference (edit: july 2020 #372). Since these options don't do anything, lockdown should fail when these options are specified.

For LavaMoat, we were surprised to find they were being provided to lockdown, but had no effect. Any other unrecognized lockdown options are rejected.

Here is a non-breaking change that adds a warning when the options are specified #2584

@kumavis
Copy link
Member Author

kumavis commented Oct 11, 2024

@kriskowal unrelated playwright install issue (?)

@kriskowal
Copy link
Member

@kriskowal unrelated playwright install issue (?)

Definitely unrelated. We saw this once before when one of the browsers shifted underneath us. I reran the job and it persisted. It’s not a required job.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

This is a breaking change. We should use a console warning instead, and leave a note somewhere for behaviors we would change to errors in the next-breaking version, maybe next year.

packages/ses/NEWS.md Outdated Show resolved Hide resolved
@kumavis
Copy link
Member Author

kumavis commented Oct 12, 2024

@kriskowal whats the process for queueing breaking changes?

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Is it unacceptable to just ignore the options?

EDIT:

My personal opinion is that this should not throw, because shipping a breaking change should be a last resort.

@kriskowal
Copy link
Member

@kriskowal whats the process for queueing breaking changes?

We do not have one yet, but I think a new label next-major is in order. The queue is not empty (__options__ on Compartment options bag, deprecated module descriptor formats, &c.)

Closing in favor of #2584

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.

3 participants