-
Notifications
You must be signed in to change notification settings - Fork 18
✨ ADR for Runtime/engine/host/environment support and CI #365
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
Open
ctcpip
wants to merge
3
commits into
master
Choose a base branch
from
env-ci
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,78 @@ | ||||||
# ADR 35580f0d-f429-412b-acef-83655e3cab11: Runtime/engine/host/environment support and CI | ||||||
|
||||||
## Status | ||||||
|
||||||
Proposed | ||||||
|
||||||
## Submitters | ||||||
|
||||||
- @ctcpip | ||||||
|
||||||
## Decision Owners | ||||||
|
||||||
- @expressjs/express-tc | ||||||
|
||||||
## Context | ||||||
|
||||||
Express and its libraries were specifically designed to run with Node.js (V8). While some of our libraries can run in other environments (e.g. runtimes, engines, browsers), they are not necessarily supported in all environments. Consequently, our CI systems do not include other environments as part of their testing workflows. | ||||||
|
||||||
Several points were raised during the discussion: | ||||||
|
||||||
- Cost of running additional CI vs the likelihood of detecting a problem | ||||||
- Introducing maintenance overhead and possible coupling to other environments' development lifecycle | ||||||
- No JS engine implements ECMAScript 100% correctly; thus, claiming "ES2015 support" does not guarantee correctness across all environments. | ||||||
- Environment regressions or language edge cases could break functionality in unpredictable ways that are not practical for us to monitor across all environments. | ||||||
|
||||||
## Decision | ||||||
|
||||||
- We will **not** add non-Node.js environment testing to our CI pipelines. | ||||||
ctcpip marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
- Exceptions may be made iff there is strongly compelling, project-aligned justification. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
- CI will continue to run only against supported Node.js versions. | ||||||
- Support for other environments may exist, per maintainer discretion, but we do not guarantee support across all environments. | ||||||
- Some libraries, particularly language-only libraries which do not require non-language APIs, strive to support as many environments as possible. | ||||||
- Nonetheless, support is not guaranteed across every possible environment, and is provided on a best-effort basis. | ||||||
- Libraries may state general compatibility information, e.g. ECMAScript version, and possibly information about supported environments, but will not explicitly list all supported environments. | ||||||
- If issues are reported for other environments, maintainers will investigate at their discretion. | ||||||
|
||||||
## Rationale | ||||||
|
||||||
- **Alternatives Considered:** | ||||||
- **Add support for additional environments in CI**: Rejected due to complexity and minimal return on value. | ||||||
|
||||||
- **Pros and Cons**: | ||||||
|
||||||
**Pros**: | ||||||
- Keeps CI lightweight and maintainable | ||||||
- Avoids implicit endorsement of non-Node.js environments | ||||||
- Maintains focus on Express's design goals and core user base | ||||||
|
||||||
**Cons**: | ||||||
- Some users may misinterpret lack of other environment testing as non-support | ||||||
- Manual verification may be required when bugs are reported in other environments | ||||||
|
||||||
- **Why is this decision the best option?** | ||||||
- It balances clarity, project focus, and contributor/maintainer effort. It avoids premature optimization or expanding scope into formal environment support. | ||||||
|
||||||
## Consequences | ||||||
|
||||||
- **Positive Impact**: | ||||||
- Reduced CI runtime and maintenance burden | ||||||
- Clearer expectations about what we support and test | ||||||
|
||||||
- **Negative Impact**: | ||||||
- Users of alternate environments may find compatibility issues undetected until runtime | ||||||
|
||||||
- **Mitigations**: | ||||||
- Update README or documentation to clarify compatibility expectations and language feature dependencies | ||||||
- Encourage users of alternate environments to report issues with enough detail to investigate | ||||||
|
||||||
## Implementation | ||||||
|
||||||
- Close PRs proposing CI additions for alternate environments unless there is strongly compelling, project-aligned justification | ||||||
- Optionally, update documentation for relevant libraries to clarify assumptions | ||||||
|
||||||
## References | ||||||
|
||||||
- [https://test262.fyi](https://test262.fyi) | ||||||
- [path-to-regexp issue on old browser support](https://github.com/pillarjs/path-to-regexp/issues/330) | ||||||
- [ADR: CommonJS and ESM](https://github.com/expressjs/discussions/pull/323) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consequence here isn't true of
path-to-regexp
, where it was instead redundant, but overall LGTM.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, happy to capture that if you have a concrete suggestion, but this is just some background context to set the stage, and don't want to risk comprehension with potentially excessive qualification and detail at this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly it looks fine already, and I think the comment about maintenance below covers the realities of
path-to-regexp
.