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] externalize middlewares.js when using adapter-node's entryPoint #2482

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

Conduitry
Copy link
Member

Fixes #2481, hopefully without breaking anything else. This is almost the same as my suggested solution in my comment on that issue - except it also includes an override for the path in the onResolve hook to avoid having import { assetsMiddleware, prerenderedMiddleware, kitMiddleware } from "../build/middlewares.js"; in the final file.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2021

🦋 Changeset detected

Latest commit: d8f380c

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-node 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

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

awesome. thanks! I'm very happy you took a look at this one as esbuild is not my forte.

I do wonder if there might be a better way to set this up. Rather than making the user import '../build/middlewares.js', perhaps we could have them do an import from @sveltejs/adapter-node? And I wonder if that might even allow us to do just a single esbuild compilation instead of the two that adapter-node is doing now

@Conduitry
Copy link
Member Author

And we would then tell esbuild to treat that specially when resolving it, and instead point it to .svelte-kit/node/middlewares.js? That's possible. It would probably be tidier, but would it be too magical? That would be a separate discussion, though, and we should fix what we have in the meantime. I think I'll open an issue shortly so we can chat about that.

@Conduitry
Copy link
Member Author

Hm. I'm having second thoughts somewhat. IIRC part of the reason for the middlewares.js thing - even before there was entryPoint- was that we wanted a way to expose middleware that could then be used as part of an entirely different build process afterwards. Does this sound familiar? We could add a nice alias for ../build/middlewares.js later as a non-breaking change, but I don't think we should stop producing that as its own build artifact if we want to give people the option to consume that however they want.

@Conduitry Conduitry merged commit 7dff79b into sveltejs:master Sep 24, 2021
@Conduitry Conduitry deleted the gh-2481 branch September 24, 2021 13:23
@benmccann
Copy link
Member

And we would then tell esbuild to treat that specially when resolving it, and instead point it to .svelte-kit/node/middlewares.js?

I was thinking it could be just a regular import from a regular node module. We might have to refactor slightly to pass in render

we wanted a way to expose middleware that could then be used as part of an entirely different build process afterwards. Does this sound familiar?

Folks wanted to be able to create a custom server and this was the solution that came out of it. I don't think anyone would miss the separate build artifact as long as they can still build a custom server

@Conduitry
Copy link
Member Author

Oops. This made middlewares.js no longer external when you're not using entryPoint, because the path to the imported file is different - it's in .svelte-kit/node/middlewares.js for regular builds. There's only an import from build/middlewares.js in custom entryPoint builds where you're pointing to that file. I'll submit a PR shortly.

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.

Error with custom entryPoint in adapter-node
2 participants