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(client-src): don't use self.location.port #1838

Merged
merged 1 commit into from May 7, 2019
Merged

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented May 1, 2019

ISSUE: #1777
REF: #1792
REF: #1664

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Revert from #1664.

Motivation / Use-Case

this pr is just a revert, but it is WIP status.(to validate)

Breaking Changes

nope

Additional Info

fixes #1777

@hiroppy
Copy link
Member Author

hiroppy commented May 1, 2019

I checked using @njugray's repository and confirmed that this problem has been fixed.

@hiroppy hiroppy marked this pull request as ready for review May 1, 2019 17:02
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Thanks, let's wait CI green

@hiroppy
Copy link
Member Author

hiroppy commented May 1, 2019

hmmm, ci is red. but since tests are passing at my local, I will investigate this issue...

@alexander-akait
Copy link
Member

/cc @hiroppy please ping me when it was done, thanks for working!

@hiroppy
Copy link
Member Author

hiroppy commented May 2, 2019

Yep, it is a difficult problem(because it just occurs on CI, and a port problem) so maybe I will use enough time.

@alexander-akait
Copy link
Member

@hiroppy friendly ping

@hiroppy hiroppy force-pushed the revert/urlParts.port branch 3 times, most recently from d979ed9 to 00aeb7b Compare May 7, 2019 15:20
@@ -62,7 +71,7 @@ describe('Client code', () => {
.waitForRequest((requestObj) => requestObj.url().match(/sockjs-node/))
Copy link
Member Author

@hiroppy hiroppy May 7, 2019

Choose a reason for hiding this comment

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

When the port is 9000, requestObj.url() is http://localhost:9000/main.js(so, match returns null) and then requestObj.url() will be http://localhost:9001/sockjs-node/info?t=xxxx after this page will be proxyed.

@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #1838 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1838   +/-   ##
=======================================
  Coverage   89.31%   89.31%           
=======================================
  Files          11       11           
  Lines         627      627           
  Branches      187      187           
=======================================
  Hits          560      560           
  Misses         55       55           
  Partials       12       12

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34058a6...9bd51e7. Read the comment docs.

@hiroppy
Copy link
Member Author

hiroppy commented May 7, 2019

@evilebottnawi PTAL

@alexander-akait
Copy link
Member

Good job, thanks!

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

Successfully merging this pull request may close these issues.

V3.3.0 live reload broken when set hostname to 0.0.0.0:xxxx
2 participants