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: Handle zstd encoding in webpack proxy config #30034

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

kgabryje
Copy link
Member

SUMMARY

Some of the latest changes caused the localhost:9000 to use zstd encoding, which we didn't handle in webpack.proxy-config.js. This PR adds simple-zstd lib to decompress zstd encoded data.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

image

After:

image

TESTING INSTRUCTIONS

Run npm run dev-server, go to localhost:9000, verify that it works normally

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@kgabryje kgabryje requested a review from villebro August 28, 2024 14:18
@kgabryje kgabryje changed the title fix: Handle zstd encoiding in webpack proxy config fix: Handle zstd encoding in webpack proxy config Aug 28, 2024
@dosubot dosubot bot added change:frontend Requires changing the frontend packages labels Aug 28, 2024
@github-actions github-actions bot removed the packages label Aug 28, 2024
@kgabryje kgabryje merged commit 75c500c into apache:master Aug 28, 2024
33 checks passed
@michael-s-molina
Copy link
Member

THANK YOU!

@villebro
Copy link
Member

Also big thanks from me! I'd be curious to understand what caused this regression..

@rusackas
Copy link
Member

Closing #29822 as superseded by this one.

@rusackas rusackas mentioned this pull request Aug 28, 2024
3 tasks
@@ -201,6 +201,7 @@
"rimraf": "^6.0.1",
"rison": "^0.1.1",
"scroll-into-view-if-needed": "^3.1.0",
"simple-zstd": "^1.4.2",
Copy link
Member

@mistercrunch mistercrunch Sep 4, 2024

Choose a reason for hiding this comment

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

@kgabryje shouldn't this be in devDependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should, I moved it to dependencies to silence eslint errors. It shouldn't really affect the entry point sizes though, as webpack will put this package in a chunk that's not downloaded on any page

@mistercrunch
Copy link
Member

mistercrunch commented Sep 4, 2024

Having issues on master RN, saying zstd not installed. Though npmjs.org seems to be having issues, with npm ci --verbose stalling for minutes at a time...

In docker-compose up

superset_node         | [webpack-cli] Failed to load '/app/superset-frontend/webpack.config.js' config
superset_node         | [webpack-cli] Error: Can not access zstd! Is it installed?

@kgabryje
Copy link
Member Author

kgabryje commented Sep 4, 2024

Having issues on master RN, saying zstd not installed. Though npmjs.org seems to be having issues, with npm ci --verbose stalling for minutes at a time...

In docker-compose up

superset_node         | [webpack-cli] Failed to load '/app/superset-frontend/webpack.config.js' config
superset_node         | [webpack-cli] Error: Can not access zstd! Is it installed?

I put apt-get install zstd in Dockerfile and it works on CI build. Do you have an idea why it's not available in docker compose?

@mistercrunch
Copy link
Member

I don't understand why it's not installed while it's specified here https://github.com/apache/superset/blob/master/Dockerfile#L35, debugging RN and had a test.js in the docker, didn't work, ran apt-get install zstd while in the container and it worked after. Very confusing.

@kgabryje
Copy link
Member Author

kgabryje commented Sep 4, 2024

I don't understand why it's not installed while it's specified here https://github.com/apache/superset/blob/master/Dockerfile#L35, debugging RN and had a test.js in the docker, didn't work, ran apt-get install zstd while in the container and it worked after. Very confusing.

Is it possible that docker compose skips the part where we install zstd in existing containers?

@mistercrunch
Copy link
Member

idk - was messing around with it and now exact same SHA command and now magically works, feels like there was a corrupt binary somewhere at Debian and now it's fixed.

@MialLewis
Copy link

MialLewis commented Sep 10, 2024

idk - was messing around with it and now exact same SHA command and now magically works, feels like there was a corrupt binary somewhere at Debian and now it's fixed.

This is happening to me... any idea what the magic fix could have been?

I've tried nuking all existing and cached containers.

@MialLewis
Copy link

MialLewis commented Sep 10, 2024

I managed to "fix" it by hacking in "dev": "apt-get update && apt-get install zstd && webpack --mode=development --color --watch"

here: https://github.com/apache/superset/blob/master/superset-frontend/package.json#L49

@mistercrunch
Copy link
Member

Really shouldn't be problem, wondering if nuking docker caches and docker compose build should get you back on your feet.

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

Successfully merging this pull request may close these issues.

7 participants