fix(remix-server-runtime): fix ServerBuild compatibility with exactOptionalPropertyTypes TS setting
#5467
Conversation
🦋 Changeset detectedLatest commit: c8a9a7a The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi @aaronadamsCA, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected]. Thanks! - The Remix team |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
|
This PR has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this PR will be automatically closed. |
github-actions
bot
added
the
needs-response
MichaelDeBoey
changed the title
ServerBuild compatibility with exactOptionalPropertyTypes TS setting
There was a problem hiding this comment.
@aaronadamsCA I guess this would not be the only place we have an optional property, so we should probably update some other types as well in that case
Don't know if this is something we want to do for all our types though 🤔
I once saw an ESLint enforcing this, but can't remember which one it was though 😕
github-actions
bot
removed
the
needs-response
Co-authored-by: Michaël De Boey <[email protected]>
Co-authored-by: Michaël De Boey <[email protected]>
You really only need it where you have an optional pass-through. IMO they can be adjusted one at a time as/if people run into challenges. |
|
Hi @aaronadamsCA! Are you still interested in getting this one merged? If so, please rebase onto latest |
MichaelDeBoey
added
the
needs-response
|
@MichaelDeBoey Done! |
github-actions
bot
removed
the
needs-response
|
I'd like to get @pcattori's eyes on this since it's both TS and dev server related |
| @@ -15,7 +15,7 @@ export interface ServerBuild { | |||
| publicPath: string; | |||
| assetsBuildDirectory: string; | |||
| future: FutureConfig; | |||
| dev?: { port: number }; | |||
There was a problem hiding this comment.
The intention is to allow omission of dev, not to let it be explicitly set to undefined. From what I can tell, including | undefined is not the semantics we want here.
Where is dev getting set to undefined explicitly? I think a better solution would be to make sure its omitted there instead.
There was a problem hiding this comment.
The problem originates here:
remix/packages/remix-dev/server-build.ts
Line 14 in 3e58915
Because ServerBuild["dev"] is an optional property, the type of dev must be widened to include undefined. You could change this type to NonNullable<ServerBuild["dev"]>, but that seems like it might be less correct than the current change.
|
|
Closes: #5466
Testing Strategy: None - I don't think there are currently any tests for this, and it's a very minor compat change, whereas new tests would be a much larger change.