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

refactor(preview)!: use base middleware #14818

Merged
merged 3 commits into from Nov 2, 2023
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 31, 2023

Description

Followup on #14756 (comment) and #14756 (comment)

Not sure if this will break a lot, but could run ecosystem-ci if this passes.

This makes it so that preview post-middlewares do not have access to the base, similar to dev as it's stripped off. For example, req.url will not have the base prepended.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

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

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Co-authored-by: 翠 / green <green@sapphi.red>
@stackblitz
Copy link

stackblitz bot commented Oct 31, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy
Copy link
Member Author

bluwy commented Oct 31, 2023

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 07793c3: Open

suite result latest scheduled
analogjs success success
astro failure success
histoire success success
ladle success success
laravel failure failure
marko failure failure
nuxt failure failure
nx failure failure
previewjs failure failure
qwik success success
rakkas success success
sveltekit failure failure
unocss success success
vike failure success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc failure success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest failure failure

@bluwy
Copy link
Member Author

bluwy commented Oct 31, 2023

Hmm it's not as bad as I thought. Seems like it's only Astro and Vike affected like before. cc @brillout again if you have thoughts on this.

There's still quite a number of usage with post-middlewares in the wild (github search). But the rest internal apps are less likely to fiddle with preview servers I think.

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks!

@sapphi-red
Copy link
Member

I think it's worth giving a try.

There's still quite a number of usage with post-middlewares in the wild (github search). But the rest internal apps are less likely to fiddle with preview servers I think.

60% of the usage is coming from a forked repo (non forked repo result).

sapphi-red
sapphi-red previously approved these changes Oct 31, 2023
@bluwy
Copy link
Member Author

bluwy commented Oct 31, 2023

I'll wait for another review from others or feedback from Rom before going with this, in case anyone has concerns.

@bluwy bluwy added this to the 5.0 milestone Oct 31, 2023
@patak-dev
Copy link
Member

This looks good to me, but I agree with waiting for Brillout's feedback.

@brillout
Copy link
Contributor

At first glance I've nothing to object.

Vike has extensive base tests, so it could be worth it to first fix the Vite+Vike CI before merging this. (I suspect Vike is red only because of #14836.)

@bluwy
Copy link
Member Author

bluwy commented Nov 2, 2023

/ecosystem-ci run vike

@bluwy
Copy link
Member Author

bluwy commented Nov 2, 2023

@brillout if you're fine with the comments I left for #14837 and #14836, I'd like to merge this PR today. It's the last PR for the milestone so we could request for general testing and make Vite 5 stable soon.

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 7fba282: Open

suite result latest scheduled
vike success success

@brillout
Copy link
Contributor

brillout commented Nov 2, 2023

@bluwy Sounds good 👍. (I didn't reply yet because I didn't manage to make appType: 'mpa' instead of appType: 'custom' work for the preview server yet. I'll further try it this morning and report back — ETA in a couple of hours.)

@brillout
Copy link
Contributor

brillout commented Nov 2, 2023

All green from my side, this PR LGTM.

@patak-dev patak-dev merged commit 69737f4 into main Nov 2, 2023
10 checks passed
@patak-dev patak-dev deleted the preview-base-middleware branch November 2, 2023 14:57
@brillout
Copy link
Contributor

brillout commented Nov 2, 2023

I'd like to merge this PR today [...] request for general testing

I think it could be worth it to merge(/reject) #14859 before.

@benmccann
Copy link
Collaborator

Next time the ecosystem CI is failing, please give us a heads up rather than ignoring it.

I really think this PR was a bad idea and if we wanted to standardize dev and preview it would have been better to do it in the reverse direction and remove the base middleware from dev mode. The base middleware is a bad actor that screws with the URL and only lets some requests through to be handled, so the only way I can get our preview mode to work is to reach in to the internals and strip out the base middleware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants