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(types): Add missing type def for isSpaMode in @remix-run/dev/server-build #8511

Closed
wants to merge 1 commit into from

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Jan 14, 2024

using remix v2.5.0, my server.ts file, which starts:

import { getAssetFromKV } from '@cloudflare/kv-asset-handler';
import { createRequestHandler, logDevReady } from '@remix-run/cloudflare';
import type { AppLoadContext } from '@remix-run/cloudflare';
import * as build from '@remix-run/dev/server-build';
import __STATIC_CONTENT_MANIFEST from '__STATIC_CONTENT_MANIFEST'; // eslint-disable-line import/no-unresolved

const MANIFEST = JSON.parse(__STATIC_CONTENT_MANIFEST);
const handleRemixRequest = createRequestHandler(build, process.env.NODE_ENV);

if (process.env.NODE_ENV === 'development') {
    logDevReady(build);
}

triggers the following type error:

server.ts:8:49 - error TS2345: Argument of type 'typeof import("/path/to/project/node_modules/@remix-run/dev/server-build")' is not assignable to parameter of type 'ServerBuild | (() => Promise<ServerBuild>)'.
  Property 'isSpaMode' is missing in type 'typeof import("/path/to/project/node_modules/@remix-run/dev/server-build")' but required in type 'ServerBuild'.

8 const handleRemixRequest = createRequestHandler(build, process.env.NODE_ENV);
                                                  ~~~~~

  node_modules/@remix-run/server-runtime/dist/build.d.ts:18:5
    18     isSpaMode: boolean;
           ~~~~~~~~~
    'isSpaMode' is declared here.

server.ts:11:17 - error TS2345: Argument of type 'typeof import("/path/to/project/node_modules/@remix-run/dev/server-build")' is not assignable to parameter of type 'ServerBuild'.

11     logDevReady(build);
                   ~~~~~


Found 2 errors in the same file, starting at: server.ts:8

looking at packages/remix-dev/server-build.ts and similar issues from the past, it seems like the new isSpaMode option just needs to be added as an export to that file to provide the type definition for the virtual module provided by the Remix compiler at build time. this is the same fix as d7a6fb5 (#4771) but for isSpaMode. as such, this PR is opened against main (like that PR), but if it should be against dev, i’m happy to update it.

Testing Strategy:

i manually edited node_modules/@remix-run/dev/server-build.d.ts like so:

--- node_modules/@remix-run/dev/server-build_unmodified.d.ts	2024-01-14 11:05:45
+++ node_modules/@remix-run/dev/server-build.d.ts	2024-01-14 11:05:56
@@ -4,5 +4,6 @@
 export declare const entry: ServerBuild["entry"];
 export declare const routes: ServerBuild["routes"];
 export declare const future: ServerBuild["future"];
+export declare const isSpaMode: ServerBuild["isSpaMode"];
 export declare const publicPath: ServerBuild["publicPath"];
 export declare const assetsBuildDirectory: ServerBuild["assetsBuildDirectory"];

and i manually edited node_modules/@remix-run/dev/server-build.js like so (shouldn’t make any different to tsc, i just wanted to be thorough with my testing):

--- node_modules/@remix-run/dev/server-build_unmodified.js	2024-01-14 11:06:01
+++ node_modules/@remix-run/dev/server-build.js	2024-01-14 11:06:07
@@ -19,6 +19,7 @@
 const entry = undefined;
 const routes = undefined;
 const future = undefined;
+const isSpaMode = undefined;
 const publicPath = undefined;
 // prettier-ignore
 const assetsBuildDirectory = undefined;

with those changes, the typescript errors were resolved.

Copy link

changeset-bot bot commented Jan 14, 2024

🦋 Changeset detected

Latest commit: 83fc881

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 14, 2024

Hi @acusti,

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 CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 14, 2024

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@acusti acusti changed the title fix(types): Add missing type def for isSpaMode in @remix-run/dev/server-build fix(types): Add missing type def for isSpaMode in @remix-run/dev/server-build Jan 14, 2024
@acusti acusti changed the title fix(types): Add missing type def for isSpaMode in @remix-run/dev/server-build fix(types): Add missing type def for isSpaMode in @remix-run/dev/server-build Jan 14, 2024
@brophdawg11
Copy link
Contributor

Thank you for the PR, but this has already been fixed in #8492 and will be available in the next release 👍

@acusti acusti deleted the patch-1 branch January 20, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants