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

docs(ses): Provide guidance/coordination for SES incompatibility tracking #1612

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

gibson042
Copy link
Contributor

Fixes #1611

@gibson042 gibson042 requested review from turadg and erights June 6, 2023 22:16
packages/ses/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

Love it!

LGTM, thanks!

packages/ses/README.md Outdated Show resolved Hide resolved
packages/ses/README.md Outdated Show resolved Hide resolved
@gibson042 gibson042 enabled auto-merge (squash) June 7, 2023 00:44
});
```

Upon encountering an incompatibility, we recommend that you add a comment to
Copy link
Member

@turadg turadg Jun 7, 2023

Choose a reason for hiding this comment

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

recommend why?

```

Upon encountering an incompatibility, we recommend that you add a comment to
[issue #576][incompatibility tracking] and file an issue with the external project referencing this section.
Copy link
Member

Choose a reason for hiding this comment

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

we could reduce the load for filing the issue by providing a template. e.g.

Some applications defend themselves by freezing their intrinsics. This project has some assignments or mutations that break in such an environment.
Specifically,
[link to source in the project]

Please consider making the project compatible with these environments by avoiding mutations to intrinsics.
If you don't have the capacity but would accept a PR, please comment to that effect so that a volunteer knows their efforts would be welcomed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, and thank you for the starting point. PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can narrow this from "assignment or mutation" to just "assignment".

by avoiding mutations to intrinsics

Is sometimes the issue and should be mentioned when relevant. But by far the most common case is using assignment to override a property inherited from a frozen intrinsic, i.e., the override mistake. We should always lead with the override mistake.

Copy link
Contributor

@erights erights Jun 7, 2023

Choose a reason for hiding this comment

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

And for the override mistake, always first suggest doing the override by defineProperty rather than assignment, because that is much more likely to be correctness preserving and least effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or rather defineProperties since that's what is already shown in this PR's example code.

@gibson042 gibson042 force-pushed the gibson-1611-ses-incompatibilities branch from 5c8b136 to f9c4283 Compare June 8, 2023 01:02
@gibson042 gibson042 force-pushed the gibson-1611-ses-incompatibilities branch from f9c4283 to 9217767 Compare June 8, 2023 01:02
@gibson042 gibson042 merged commit 08e8aa3 into master Jun 8, 2023
12 checks passed
@gibson042 gibson042 deleted the gibson-1611-ses-incompatibilities branch June 8, 2023 01:10
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.

Documentation/correction of SES-incompatible code should be coordinated
4 participants