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.

Sign up for GitHub

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

Jump to bottom

chore(build): uplift webpack-related packages to v5, 2nd attempt #28440

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

chore(build): uplift webpack-related packages to v5, 2nd attempt #28440

wants to merge 5 commits into from

Conversation

hainenber
Copy link
Contributor

@hainenber hainenber commented May 12, 2024 •
edited
Loading

chore(build): uplift webpack-related packages to v5

SUMMARY

2nd attempt of #28342 with implemented solution from @michael-s-molina (thank you very much!)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

npm run dev-server results

  • From CLI
image
  • From browser
image

npm run build and npm run dev results are OK without issues

TESTING INSTRUCTIONS

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

@hainenber
chore(build): uplift webpack-related packages to v5
4734d0a 
Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber hainenber changed the title chore(build): uplift webpack-related packages to v5 chore(build): uplift webpack-related packages to v5, 2nd attempt May 12, 2024
@michael-s-molina
Copy link
Member

Hi @hainenber. It's not working for me. Did you run npm ci before running npm run dev-server?

Screen.Recording.2024-05-13.at.08.32.55.mov

@hainenber
fix(frontend/dev-server): proxy to IPv4-form Superset backend
66c3f76 
Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber
Copy link
Contributor Author

hainenber commented May 13, 2024 •
edited
Loading

@michael-s-molina Yes I can confirm the issue not yet resolved.
I've digged around and it appears that this is the culprit and I've fixed it in the HEAD commit.

Since IPv4 is still supported for probably eternity in localhost settings, I think the change isn't too hurtful (unless there are radical folks with IPv6-only stack :D)

Can you take another look? Thanks!

rusackas
rusackas reviewed May 13, 2024
View reviewed changes
superset-frontend/webpack.proxy-config.js Outdated
@@ -30,7 +30,7 @@ const parsedEnvArg = () => {
};

const { supersetPort = 8088, superset: supersetUrl = null } = parsedEnvArg();
const backend = (supersetUrl || `http://localhost:${supersetPort}`).replace(
Copy link
Member

@rusackas rusackas May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this fixing anything? Someone might configure localhost to be at another IP, so localhost might be (marginally) safer.

Copy link
Member

Choose a reason for hiding this comment

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

#28440 (comment)

Copy link
Member

@rusackas rusackas May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hah! The answer was right there! 🤦 What a goofy change for them to make.

I wonder if forcing IPV4 like this is risky. Some might change that IP, or some might want IPV6 support.

Maybe it's worth a big find/replace to use "::" rather than "0.0.0.0" everywhere, and "127.0.0.1" to "::1", so that it'll (supposedly) support both?

Copy link
Contributor Author

@hainenber hainenber May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I dug deeper into the rabbit hole and Node 20 should be able to fix the IPv6-over-IPv4 issues automatically, thanks to this PR.

I tried reverting the localhost -> 127.0.0.1 change, nvm use 20, run npm ci && npm run dev-server and voila, localhost:9000 is proxied properly!

So yeah, this change can wait until Node20 is chosen for superset-frontend's .nvmrc :D

Copy link
Member

Choose a reason for hiding this comment

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

Good to know! @michael-s-molina and I were talking about making this bump as part of the 5.0 release rather than rocking the boat beforehand. I'll try to open a PR with Node 20 sooner rather than later, and just make sure we don't merge it too soon :)

@hainenber
Merge branch 'master' into uplift-webpack-related-packages-to-v5-2nd-…
91c0f1b 
…time

Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber
fix(ci): revert localhost to domain instead of hardcoded IPv4
65d3e3b 
Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber
fix(ci): revert to v2 lockfile
a6cf982 
Signed-off-by: hainenber <dotronghai96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rusackas rusackas rusackas left review comments

@kgabryje kgabryje Awaiting requested review from kgabryje

@michael-s-molina michael-s-molina Awaiting requested review from michael-s-molina

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
dependencies:npm embedded size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hainenber @michael-s-molina @rusackas