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

Fixes #970 by adding middleware dev config option. #1062

Merged
merged 2 commits into from
Sep 26, 2020
Merged

Fixes #970 by adding middleware dev config option. #1062

merged 2 commits into from
Sep 26, 2020

Conversation

joshwilsonvu
Copy link
Contributor

Changes

Hello,

As an alternative to exposing the Snowpack dev server as Express-like middleware, I think we
could accomplish the same goal by adding a middleware dev option in snowpack.config.js.
It would accept a (req, res, next) => void function to allow users to intercept requests before
they reach the dev server. This way, the logic is inverted and Snowpack is able to "drive" --
users don't have to worry about HMR, websockets, etc.

For example, I could handle /api/route like so:

// snowpack.config.js
const express = require('express');
const app = express();
app.get('/api/route', (req, res) => {
  res.json({
    some: 'json',
  });
});

module.exports = {
  mount: {
    public: '/',
    src: '/_dist_',
  },
  plugins: [],
  devOptions: {
    middleware: app,
  },
};

Testing

I tested my changes by using the code above and altering an app created with
app-template-blank to fetch /api/route and log the response. I got
{ "some": "json" } in the console and the rest of the page worked unchanged.

I'm happy to add a test, but this affects the dev server only and I haven't come across
testing for that. Feel free to point me in the right direction.

@vercel
Copy link

vercel bot commented Sep 15, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/88j3vft3z
✅ Preview: https://snowpack-git-fork-joshwilsonvu-master.pikapkg.vercel.app

@FredKSchott
Copy link
Owner

Thanks for the PR! +1 on the overall idea here, but we may want to think this through a bit first.

also /cc @Rich-Harris any interest in this feature on your end?

A couple of thoughts:

  • We currently have the proxy config to proxy requests to an existing server during dev.
  • A footgun that I want to avoid is letting devs modify assets (JS, CSS, etc.) through the server and circumventing the build pipeline. Handling non-asset responses (APIs, SSR, etc) is fine, is there a way that we could enforce that? Only calling the user-provided server (app) for non-asset routes?

@joshwilsonvu
Copy link
Contributor Author

joshwilsonvu commented Sep 20, 2020

Of course, it's just an idea. I agree that proxying a subtree of requests is the idiomatic way to go here, I just have legacy code outside of my control where that doesn't work due to strange design. Let's only merge this if others find it useful.

In regards to the footgun you mentioned, there's no way for middleware to alter or even access the dev server assets (without explicitly sending a separate HTTP request to the dev server). However, I do see a danger of middleware routes unintentionally shadowing asset URLs. If there's some way to put the middleware "behind" the dev server, that problem would be mitigated, but Snowpack currently sends index.html to requests that don't match any asset. That would completely shadow the middleware.

What if we reserved the /api subtree for middleware? All routes defined in the middleware could be handled relative to /api, and there would be no chance of shadowing. This ultimately wouldn't be much different from proxying, but it wouldn't require a separate server and maybe it could satisfy those who want a Snowpack Express middleware. It might cause confusion though, or conflict with a directory mounted to /api.

@FredKSchott
Copy link
Owner

That's a great point, this would come BEFORE the normal build pipeline, not after. So in this implementation, that concern of mine isn't actually real.

huh, this may actually work best exactly as is, which is exciting because it looks so simple!

/cc @drwpow any thoughts on this?

@FredKSchott
Copy link
Owner

Okay, lets do it. I feel really good about this direction, especially given the new interest in Snowpack's SSR story.

I may rename to experiment.middleware before we launch so that we can give this some time to collect feedback before committing to the final API.

Thanks again for introducing this!

@FredKSchott FredKSchott merged commit eba63e3 into FredKSchott:master Sep 26, 2020
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.

None yet

2 participants