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: replacing compression with modern version #6557

Merged
merged 2 commits into from
Mar 8, 2022

Conversation

Niputi
Copy link
Contributor

@Niputi Niputi commented Jan 18, 2022

Description

correctly fix: #2754

once lukeed/polka#148 have merged and released as a polka package, we can use their package instead.
code is copied and slightly modified from the pull request

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 Commit 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.

@Niputi Niputi changed the title replacing compression with modern version fix: replacing compression with modern version Jan 18, 2022
@Niputi
Copy link
Contributor Author

Niputi commented Jan 18, 2022

added compression to the root as I don't believe it's possible to import vite middlewares in playground ssr-vue & ssr-react

@TrySound
Copy link
Contributor

Btw can you check rm -rf dist/node && pnpm run build-bundle && du -ck dist/node on main and in your branch?

@Niputi Niputi requested a review from patak-dev January 26, 2022 13:20
@patak-dev
Copy link
Member

@Niputi in the PR developit mentions a fix in the inlined version of WMR of it that wasn't backported. Did you check what that was about? lukeed/polka#148 (comment)

@Niputi
Copy link
Contributor Author

Niputi commented Jan 26, 2022

I can see a few differences when comparing this to https://github.com/preactjs/wmr/pull/155/files
will update later to match with the wmr pull request

@patak-dev
Copy link
Member

@Niputi maybe you could directly copy the current version from WMR adding their license in it: https://github.com/preactjs/wmr/blob/main/packages/wmr/src/lib/polkompress.js

There are other changes it seems compared to the one in your last comment.

@Niputi Niputi force-pushed the feat/compression-mordern-version branch from 30cb9d6 to 82b3c99 Compare January 31, 2022 22:46
@Niputi Niputi added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jan 31, 2022
@Niputi
Copy link
Contributor Author

Niputi commented Feb 2, 2022

@patak-dev is this change good?

@patak-dev
Copy link
Member

I think it is good @Niputi, thanks for updating it! Let's wait for the next minor for it so we more testing. I think we should stop merging new changes until we release 2.8. We should start 2.9 rather soon and avoid being one month again in beta

@patak-dev patak-dev added this to the 2.9 milestone Feb 2, 2022
@patak-dev
Copy link
Member

@Niputi would you resolve the conflicts so we merge this PR? Thanks!

@Niputi Niputi force-pushed the feat/compression-mordern-version branch from b85294d to 1e45e37 Compare March 5, 2022 21:39
@Niputi
Copy link
Contributor Author

Niputi commented Mar 5, 2022

done @patak-dev

package.json Outdated Show resolved Hide resolved
@patak-dev patak-dev requested a review from bluwy March 7, 2022 21:30
@patak-dev patak-dev merged commit 5648d09 into vitejs:main Mar 8, 2022
@Niputi Niputi deleted the feat/compression-mordern-version branch March 8, 2022 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
No open projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

this._implicitHeader is not a function
4 participants