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

add vite preset #14

Merged
merged 8 commits into from
Oct 9, 2024
Merged

add vite preset #14

merged 8 commits into from
Oct 9, 2024

Conversation

wingleung
Copy link
Owner

closes #8

Added a Vite preset that will

  • add a step after the remix build
  • do an esbuild of a server.js which uses the createRequestHandler from remix-aws
  • overwrites the remix server build with the one build bij esbuild

You can provide your own server.js by providing your own build.entryPoints.
You can provide your own build.outfille if you don't want to overwrite the remix server build.

Check the readme for configuration details

@wingleung wingleung self-assigned this Apr 20, 2024
@brettdh
Copy link
Contributor

brettdh commented Oct 3, 2024

@wingleung are you still planning to actively maintain this project, or are you using something else nowadays?

I'm planning to upgrade to Remix + Vite soon, and I'd love to see this get merged, and to see #44 get fixed. (I have a patch, but I haven't made a PR because the project seems stalled.)

@wingleung
Copy link
Owner Author

wingleung commented Oct 4, 2024

@wingleung are you still planning to actively maintain this project, or are you using something else nowadays?

@brettdh apologies for the lack of activity on my part, it's been a busy. We're still using remix v2 with vite and our internal PR is still open to test the vite plugin 😞 I'll make sure to make to some time to test the vite plugin and be more active in this project the coming days 🙏 keep you posted!

@meza
Copy link

meza commented Oct 5, 2024

Really looking forward to this one! Can I help somehow?

}
)
```

Copy link

Choose a reason for hiding this comment

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

Would be nice to have a section on local testing. What script targets would be appropriate to use?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@meza good idea!

I usually test this with sam cli so I can reproduce the same infrastructure as I would use in AWS.
I created a simple remix example project where I added some readme, predefined npm scripts and already configured the vite preset for testing. Please check the readme if you want to give it a spin.

👉 https://github.com/wingleung/remix-aws-example
👉 especially this commit where I added the remix-aws setup

Copy link

Choose a reason for hiding this comment

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

thanks! Also is there a reason to stay with awsgateway v1?

Copy link
Owner Author

Choose a reason for hiding this comment

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

good point, I added a commit to make api gw v2 the default everywhere 👉 e5fdefb

it was already the default in the vite preset and in the createRequestHandler and the vite preset

@wingleung wingleung force-pushed the add-vite-support branch 2 times, most recently from 2d8f26c to 2afcbbe Compare October 5, 2024 22:54
@wingleung
Copy link
Owner Author

@brettdh @meza I published a beta version which I use internally during our testing period for this, feel free to have a look.
https://www.npmjs.com/package/remix-aws/v/1.2.0-beta.2

@meza
Copy link

meza commented Oct 6, 2024

Awesome! About to dive in. Will report back when I got somewhere, thanks!

@meza
Copy link

meza commented Oct 6, 2024

Aside from this warning, it seems to be working so far locally. About to push it through CDK and prod :)

image

@brettdh
Copy link
Contributor

brettdh commented Oct 7, 2024

@brettdh @meza I published a beta version which I use internally during our testing period for this, feel free to have a look. https://www.npmjs.com/package/remix-aws/v/1.2.0-beta.2

Thanks for the update and the beta! Does this release contain the fix for #44 or not yet? (Just checking before I remove my local patch.)

@brettdh
Copy link
Contributor

brettdh commented Oct 7, 2024

Looking at this again today, I'm actually not sure if I should use a preset. In my custom server.ts that I currently use (before migrating to Vite), I have this:

export const handler = initSentry(
  handleWarmup(
    ignoreBadRequestErrors(
      createRequestHandler({
        build,
        mode: process.env.NODE_ENV,
        awsProxy: AWSProxy.APIGatewayV2,
      }) as AsyncHandler<APIGatewayProxyHandler>,
    ) as AsyncHandler<APIGatewayProxyHandler>,
  ) as AsyncHandler<APIGatewayProxyHandler>,
)

initSentry, handleWarmup, and ignoreBadRequestErrors are wrappers for the request handler from remix-aws that do what their names suggest. Is there a way to still apply these wrappers when using the preset, or at that point is it better to avoid the preset and configure Vite directly?

@brettdh
Copy link
Contributor

brettdh commented Oct 7, 2024

Ope I should read more carefully:

You can provide your own server.js by providing your own build.entryPoints.

Will try this.

@wingleung wingleung changed the base branch from main to develop October 7, 2024 18:59
@brettdh
Copy link
Contributor

brettdh commented Oct 7, 2024

One issue: it looks like my overriding of serverBuildFile is not having an effect... I pass this as my preset but the resulting file is build/server/index.js:

      awsPreset({
        build: {
          entryPoints: ["server.ts"],
        },
        serverBuildFile: "index.mjs",
      }),

@meza
Copy link

meza commented Oct 7, 2024

Btw I've managed to make it all work with CDK. I'll distill it down and update the Trance Stack (which is well overdue) to all of what I've learned

@wingleung
Copy link
Owner Author

One issue: it looks like my overriding of serverBuildFile is not having an effect... I pass this as my preset but the resulting file is build/server/index.js:

      awsPreset({
        build: {
          entryPoints: ["server.ts"],
        },
        serverBuildFile: "index.mjs",
      }),

@brettdh hmm I should pass the outfile config based on the remix config by default here 👉 https://github.com/wingleung/remix-aws/blob/add-vite-support/src/vite-preset.ts#L34

I'll work on that, for now you could provide your own outfile in the build config

      awsPreset({
        build: {
          entryPoints: ["server.ts"],
          outfile: "build/server/index.mjs"
        },
        serverBuildFile: "index.mjs",
      }),

@wingleung
Copy link
Owner Author

wingleung commented Oct 7, 2024

@brettdh I think you added the serverBuildFile in the wrong config, it should be on the remix root config.
I added a commit that will configure a default outfile based on the remix config automatically 👉 3e8da0d

I'll try to make a beta release tomorrow morning with the latest commits of this PR and of #44 🙏

export default defineConfig({
  plugins: [
    remix({
      serverBuildFile: 'index.mjs',
      presets: [
        awsPreset(
          {
            awsProxy: AWSProxy.APIGatewayV1,
          }
        ) as Preset
      ],
    }),

@brettdh
Copy link
Contributor

brettdh commented Oct 7, 2024

I also had it there but looking at the PR diff, it seemed like I also had to pass it as part of the preset config; otherwise it would use the default path of build/server/index.js. I'll take another look look later today.

@wingleung
Copy link
Owner Author

@brettdh I think I need to work on better documentation or self explanatory code 😅 the config for the preset itself only takes 2 config properties at the root

  • awsProxy: to configure which proxy to use, apigwv1, apigwv2,...
  • build: to configure the esbuild step in the onBuildEnd hook of remix

Default config
Screenshot 2024-10-07 at 21 42 09

in the onBuildEnd hook we receive a remixConfig from remix where we can use the buildDirectory and serverBuildFile from the remix config to concatenate our own outfile config
👉 https://github.com/remix-run/remix/blob/main/packages/remix-dev/vite/plugin.ts#L244

@brettdh
Copy link
Contributor

brettdh commented Oct 7, 2024

Ah, okay; thanks for the explanation. This was the crucial bit I was misunderstanding (emphasis mine):

in the onBuildEnd hook we receive a remixConfig from remix

I was misreading some sequence of calls and thinking that the remixConfig in buildEndHandler was coming from the arguments to awsProxy somehow.

...aaaaalso it seems I was passing the result of awsPreset as a Vite plugin rather than in the presets arg to the Remix Vite plugin. 😅 I feel like Typescript let me down somehow here but also I just made a silly mistake.

Moving everything to the right spot and adding outfile as you suggested did indeed produce build/server/index.mjs. Oddly, though, it also produced index.js and index.js.map in that directory, but no index.mjs.map, so it seems like something is still a bit off.

@brettdh
Copy link
Contributor

brettdh commented Oct 8, 2024

Not quite sure if this is related to remix-aws or not, but I now have this issue:

ReferenceError: module is not defined in ES module scope
        "    at file:///var/task/ui/build/server/index.mjs:305908:1",

The referenced line is the last one in this block:

// server.ts
var server_exports2 = {};
__export(server_exports2, {
  handler: () => handler
});
module.exports = __toCommonJS(server_exports2);   // <-- this line
My `vite.config.ts` file
import { vitePlugin as remix, VitePluginConfig } from "@remix-run/dev"
import { installGlobals } from "@remix-run/node"
import { awsPreset } from "remix-aws"
import { remixDevTools } from "remix-development-tools"
import { defineConfig } from "vite"
import type { UserConfig } from "vite"
import tsconfigPaths from "vite-tsconfig-paths"

import { getStaticAssetsBaseUrl } from "./utils.js"

installGlobals()

const isProd = process.env.NODE_ENV === "production"

const remixProdConfig: Partial<
  Pick<VitePluginConfig, "serverBuildFile" | "presets">
> = isProd
  ? {
      serverBuildFile: "index.mjs",
      presets: [
        awsPreset({
          build: {
            entryPoints: ["server.ts"],
            outfile: "build/server/index.mjs",
          },
        }),
      ],
    }
  : {}

const viteProdConfig: Partial<Pick<UserConfig, "base" | "ssr">> = isProd
  ? {
      base: `${getStaticAssetsBaseUrl()}/static/build`,
      ssr: {
        noExternal: true,
      },
    }
  : {}

export default defineConfig({
  build: {
    sourcemap: true,
    commonjsOptions: {
      transformMixedEsModules: true,   // I tried this based on a google search result but it didn't help
    },
  },
  ...viteProdConfig,
  server: {
    port: 3000,
  },
  plugins: [
    remixDevTools(),
    remix({
      ignoredRouteFiles: ["**/.*", "**/*.css", "**/*.test.{js,jsx,ts,tsx}"],
      ...remixProdConfig,
    }),
    tsconfigPaths(),
  ],
})
My `server.ts` file
import { installGlobals } from "@remix-run/node"
import * as Sentry from "@sentry/node"
import type { APIGatewayProxyHandler } from "aws-lambda"
import type { AsyncHandler } from "aws-lambda-consumer"
import { AWSProxy, createRequestHandler } from "remix-aws"
import sourceMapSupport from "source-map-support"

import { initSentry } from "~/lib/sentry.server"

// eslint-disable-next-line @typescript-eslint/no-var-requires
const build = require("./build/server/index")

if (process.env.NODE_ENV !== "production") {
  require("./mocks")
}

installGlobals()
sourceMapSupport.install()

function handleWarmup(handler: AsyncHandler<APIGatewayProxyHandler>) {
  // <snip>
}

/**
 * Prevent Sentry trace reporting for all client-side errors in the UI.
 *
 * These are most commonly the result of scanner traffic, but even for
 * invalid requests that occur during development, we don't particularly
 * care about recording traces.
 */
function ignoreBadRequestErrors(handler: AsyncHandler<APIGatewayProxyHandler>) {
  // <snip>
}

export const handler = initSentry(
  handleWarmup(
    ignoreBadRequestErrors(
      createRequestHandler({
        build,
        mode: process.env.NODE_ENV,
        awsProxy: AWSProxy.APIGatewayV2,
      }) as AsyncHandler<APIGatewayProxyHandler>,
    ) as AsyncHandler<APIGatewayProxyHandler>,
  ) as AsyncHandler<APIGatewayProxyHandler>,
)

@brettdh
Copy link
Contributor

brettdh commented Oct 8, 2024

Figured it out; I needed to add these lines to the preset's esbuild config:

            // This fixed the `module is not defined` issue above
            format: "esm",

            // These lines applied a workaround I'd needed previously, but now in the Vite context
            // https://github.com/evanw/esbuild/issues/1921#issuecomment-2302290651
            mainFields: ["module", "main"],
            banner: {
              js: "import { createRequire } from 'module'; const require = createRequire(import.meta.url);",
            },

@brettdh
Copy link
Contributor

brettdh commented Oct 8, 2024

Anything special I need to do to make this work with the vite base setting? I'm setting it to a Cloudfront URL to serve the CSS and Javascript that's built in my Remix app, but the sources for those in the HTML are still /assets/... for some reason.

@brettdh
Copy link
Contributor

brettdh commented Oct 8, 2024

Nevermind; there's something unexpected (to me) going on here where my vite.config.ts is executed multiple times during the course of a remix vite:build, with NODE_ENV being production at first but then later being development.

Update: figured this out by passing a function to defineConfig, which accepts ({ mode }), where mode is always production during a build.

@brettdh
Copy link
Contributor

brettdh commented Oct 9, 2024

@wingleung something about the preset is messing up my server source maps. I'm using a variant of this script to check the source mapping of a particular line in my bundle, and when I remove the awsPreset(...) entry in the presets array, the source map lookup is correct, but when I add the awsPreset(...) entry back, the lookup fails to find a mapping back to any source file.

@brettdh
Copy link
Contributor

brettdh commented Oct 9, 2024

Happily, all the other testing I've done is working now 🥳 just the issue with the server bundle source maps remains.

@wingleung wingleung merged commit a30af9c into develop Oct 9, 2024
@wingleung wingleung deleted the add-vite-support branch October 9, 2024 20:32
@wingleung wingleung restored the add-vite-support branch October 9, 2024 20:32
@wingleung
Copy link
Owner Author

@brettdh first of all, thank you so much for taking the time to test this beta version 🙏 much appreciated!

Figured it out; I needed to add these lines to the preset's esbuild config:

            // This fixed the `module is not defined` issue above
            format: "esm",

            // These lines applied a workaround I'd needed previously, but now in the Vite context
            // https://github.com/evanw/esbuild/issues/1921#issuecomment-2302290651
            mainFields: ["module", "main"],
            banner: {
              js: "import { createRequire } from 'module'; const require = createRequire(import.meta.url);",
            },

☝️ the esbuild that is triggered by remix-aws should be more intelligent here and map the remix' vite config to esbuild, pushed a commit to map some more build configs to esbuild 👉 a05e6db

Anything special I need to do to make this work with the vite base setting? I'm setting it to a Cloudfront URL to serve the CSS and Javascript that's built in my Remix app, but the sources for those in the HTML are still /assets/... for some reason.

☝️ this is another example why remix-aws should be more intelligent by mapping the current build config to esbuild in the onBuildEnd hook.

I mapped the base config from vite to the publicPath config in esbuild. however, when I add a url it still adds /assets to the paths, remix-aws doesn't do anything with assets so I need to check if this is maybe a bug in remix 🤔 according to the docs the default should be /

publicPath has been replaced by Vite's "base" option which defaults to "/" rather than "/build/".

docs: https://remix.run/docs/en/main/guides/vite#new-build-output-paths

@wingleung something about the preset is messing up my server source maps. I'm using a variant of this script to check the source mapping of a particular line in my bundle, and when I remove the awsPreset(...) entry in the presets array, the source map lookup is correct, but when I add the awsPreset(...) entry back, the lookup fails to find a mapping back to any source file.

☝️ it might be possible sourcemap or minify was not in sync with how remix' vite was configured, just to be sure I mapped those as well in a05e6db
I also added a quick test in the example app 👉 wingleung/remix-aws-example@8e00d9e

📢 I published a new beta version with the added mappings for you to test it on your setup 👉 1.2.0-beta.3

feel free to add more feedback here, I merged the PR so I could have a clean branching strategy to add multiple PRs into beta's of an upcoming version

@brettdh
Copy link
Contributor

brettdh commented Oct 10, 2024

Installed 1.2.0-beta.3 and the server sourcemaps work perfectly. Thank you!!

@brettdh
Copy link
Contributor

brettdh commented Oct 10, 2024

I added a few comments on a05e6db but they're pretty minor. Is there more testing or PR merging you want to do before releasing 1.2.0, or do you think that might happen pretty soon?

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.

HOWTO: Usage with remix vite plugin
3 participants