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

Document existing use of env property #7

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

Conversation

jkrems
Copy link

@jkrems jkrems commented Oct 1, 2024

Alternative to #5 that focusses on documenting the status quo instead of suggesting a specific spec for the env field.

I'm explicitly not using "environment variables" in the description because in Vite it contains things like .DEV/.PROD which are booleans and very much not environment variables. See also: semantics are currently inconsistent and not fully interoperable.

Happy to add a "please don't add unflagged in more places until semantics are specified", to me the "Warning" already implies that.

@guybedford
Copy link

I wonder if we need a "status" column in this table where we distinguish between "known convention" and "experimental" and "recommended standard"? This seems like a place where explicitly documenting this as a convention rather than a recommended standard could be useful.

@jkrems
Copy link
Author

jkrems commented Oct 6, 2024

I added a proposed "status" column that expresses two dimensions:

  1. How well are the semantics of this field specified?
  2. How "standard" is it to have this field in a given runtime?

README.md Outdated
* 🛑 **Discouraged:** There's a better, more interoperable alternative to using this field that should be used instead.

Stability level:
* 🪨 **Stable:** Field has a clear specification and tests to ensure interoperable behavior. Deviation from the specified behavior is likely a bug in a runtime.

Choose a reason for hiding this comment

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

On Windows this emoji is coming up as unknown for me currently.

Copy link
Author

Choose a reason for hiding this comment

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

Switched to 🚀.

README.md Outdated
Comment on lines 28 to 29
* ✅ **Baseline:** Code can generally expect this field to be present and runtimes should generally implement it.
* ☑️ **Optional:** The field may be unavailable in certain environments. Code should handle the field's absence to be fully portable.

Choose a reason for hiding this comment

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

Maybe these can be Baseline standard and Optional standard?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good!

@jkrems
Copy link
Author

jkrems commented Oct 14, 2024

@ljharb Any thoughts on this one? It has no objections yet but the bit missing is an approval from a wintercg member.

@guybedford
Copy link

I wonder if it would be worth adding this to the meeting agenda?

@jkrems
Copy link
Author

jkrems commented Oct 14, 2024

I'm also starting to suspect that I may be misunderstanding the goals of the registry. I was looking for a place that documents meta properties that are in active use to identify potential interop risks etc.. But from the PR comments, it doesn't sound like that's what this registry is..?

If the registry is just for "recommended & interoperable standard properties across WinterCG-aligned runtimes", then this PR might just be sent to the wrong repo. :)

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