Skip to content

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

Closed
hainenber wants to merge 5 commits into
apache:masterfrom
hainenber:uplift-webpack-related-packages-to-v5-2nd-time
Closed

chore(build): uplift webpack-related packages to v5, 2nd attempt#28440
hainenber wants to merge 5 commits into
apache:masterfrom
hainenber:uplift-webpack-related-packages-to-v5-2nd-time

Conversation

@hainenber

@hainenber hainenber commented May 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

michael-s-molina commented May 13, 2024

Copy link
Copy Markdown
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

hainenber commented May 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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
Comment thread superset-frontend/webpack.proxy-config.js
};

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

@rusackas rusackas May 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@michael-s-molina michael-s-molina May 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#28440 (comment)

@rusackas rusackas May 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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?

@hainenber hainenber May 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@rusackas rusackas May 14, 2024

Copy link
Copy Markdown
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>
@hainenber

hainenber commented Oct 5, 2024

Copy link
Copy Markdown
Contributor Author

Closing this for a newer one checked out from latest master branch

@hainenber hainenber closed this Oct 5, 2024
@hainenber hainenber deleted the uplift-webpack-related-packages-to-v5-2nd-time branch October 5, 2024 17:29
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

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