Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Add ignore option #326

Merged
merged 4 commits into from
Aug 1, 2018
Merged

Add ignore option #326

merged 4 commits into from
Aug 1, 2018

Conversation

lukeed
Copy link
Member

@lukeed lukeed commented Jul 29, 2018

When Sapper runs as a top-level middleware, it (strongly) assumes it's the final handler for any/all requests on that domain. While this is 100% desired in most cases (and especially when relegated to a basepath), it does make it near-impossible to prevent this behavior in the 1-2% of cases where an escape hatch is needed.

More specifically, Polka users are forced to run everything as a root-level middleware, even if a sub-application is meant to control its own basepath. This is currently the only work around in this case:

polka()
  .use(
    compression(),
    // ...
    (req, res, next) => {
      if (req.path.startsWith('/users') {
        req.path = req.path.substring(6); // remove "/users"
        return users_sub_application(req, res, next);
      }
      next();
    },
    // ...
    sapper()
  )

With this feature, Polka (or any) users can bypass Sapper's routing and allow their legacy/sub/third-party/whatever apps to run on the same server, without worrying about middleware sequencing.

The options.ignore can be an instance of RegExp, Function, String, or an Array of any combination.

sapper({
  store,
  manifest,
  ignore: [
    /foobar/i,
   'foobar', // leading slash will be added
    '/foobar', // identical
    uri => uri.startsWith('/foobar')
  ]
});

In the middleware composition, a new middleware is added only if/when the options.ignore is defined. The req.__SAPPER__IGNORE__ boolean will be set & is used in the following middlewares to exit early, avoiding any sapper-related work that isn't desired for the current, incoming request.

@Rich-Harris
Copy link
Member

Thanks — I still strongly feel that Polka should adopt Express's behaviour here (partly because it's just much easier to understand, but also because it would make it simpler to migrate existing Express apps), but in lieu of that...!

Before I merge, any idea what's going on with the test failures?

@lukeed
Copy link
Member Author

lukeed commented Jul 31, 2018

This is up to you, of course! As mentioned, there is an avenue to deal with this currently, it's just not super ideal.

While I (now) understand your stance, unfortunately it changes the entirety of Polka. It might be possible to do before the 1.0 release (or a future major), but it does require a full rewrite :/

No idea on the test failures. I didn't even look into what it's doing tbh. That one passes locally, but a bunch of others fail here (cookie management & whatnot). Maybe rerunning both test suites will do the trick? lol 😅

@Rich-Harris
Copy link
Member

super weird — when I checkout this branch locally, I get the same errors as Travis and Appveyor. Can't figure out why it'd be related to these changes (though i haven't looked deeply yet). Will have to investigate further when i'm not at work

@lukeed
Copy link
Member Author

lukeed commented Jul 31, 2018

I'll try to do the same tonight or tomorrow night, including fresh installs.

Sorry for the trouble

@lukeed
Copy link
Member Author

lukeed commented Jul 31, 2018

Ah, found it 😆 The failing test navigates to /foo/bar/baz which I've marked as a route to ignore. My "custom" routes are just sending down plaintext, so it's competing with one another.

Fix incoming~

~> leaked into another test’s route
@Rich-Harris Rich-Harris merged commit 47a6d6f into sveltejs:master Aug 1, 2018
@Rich-Harris
Copy link
Member

thanks! released 0.15.4. Opened an issue on the docs repo — not exactly sure where to put it https://github.com/sveltejs/sapper.svelte.technology/issues/24

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants