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

Sockjs prefix config #911

Merged
merged 7 commits into from
Aug 8, 2017
Merged

Conversation

kellyrmilligan
Copy link
Contributor

What kind of change does this PR introduce?
This adds the capability to prefix the url that sockjs-node is served from. This is useful for having multiple applications in development being behind a reverse proxy.

Did you add or update the examples/?

Summary

#909

Does this PR introduce a breaking change?

Other information

@jsf-clabot
Copy link

jsf-clabot commented May 18, 2017

CLA assistant check
All committers have signed the CLA.

@kellyrmilligan
Copy link
Contributor Author

I need some guidance, in index.js for the client that gets built, i'm wondering

var socketUrl = url.format({
	protocol: protocol,
	auth: urlParts.auth,
	hostname: hostname,
	port: (urlParts.port === "0") ? self.location.port : urlParts.port,
	pathname: urlParts.path == null || urlParts.path === "/" ? "/sockjs-node" : urlParts.path
});

in what scenario does urlParts.path get used?

what other scenarios involve /sockjs-node on the client-side?

@kellyrmilligan
Copy link
Contributor Author

updated to take a stab at this. it seems like the urlParts.path woudl be used when you pass a query string while including the hot client file. I am not sure this change needs to handle that scenario?

@kellyrmilligan
Copy link
Contributor Author

also added an example doing it, although I can't seem to figure out adding historyapifallback into the mix to fully show a subapp. any ideas?

@codecov
Copy link

codecov bot commented May 19, 2017

Codecov Report

Merging #911 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
+ Coverage   72.25%   72.31%   +0.05%     
==========================================
  Files           4        4              
  Lines         465      466       +1     
  Branches      140      141       +1     
==========================================
+ Hits          336      337       +1     
  Misses        129      129
Impacted Files Coverage Δ
lib/Server.js 79.87% <100%> (+0.06%) ⬆️

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 1cf4359...4d87a6d. Read the comment docs.

@kellyrmilligan
Copy link
Contributor Author

@SpaceK33z what are your thoughts on this?

facebook/create-react-app#1887

depends on this being in place.

@Timer
Copy link

Timer commented Jul 26, 2017

Pinging @d3viant0ne; we'd love to see some feedback/progression on this PR so we can land facebook/create-react-app#1887.

@Timer
Copy link

Timer commented Jul 27, 2017

@kellyrmilligan would you be available to bring this up to date if we can get the ball rolling?

@kellyrmilligan
Copy link
Contributor Author

Yes I can spend some time on it for sure. Been meaning to try and more directly ping someone so thanks for that

@joshwiens
Copy link
Member

@kellyrmilligan - Get it rebased if you don't mind and we'll get it queued up.

@kellyrmilligan
Copy link
Contributor Author

🤘

@kellyrmilligan
Copy link
Contributor Author

@d3viant0ne @Timer updated to latest master. I've only been a consumer of webpack dev server in the past, so let me preface this pr with that. I am not sure i'm using the proper variable in the client side portion of this to accommodate the prefix.

@joshwiens
Copy link
Member

FYI - This hasn't been forgotten, most of the people that can pick this up & actually do something with it are all busy i.r.l. I was hoping to get this done without having to ping Kees but it's looking like that's the way it's going to have to be.

@shellscape shellscape merged commit ccd113a into webpack:master Aug 8, 2017
@shellscape
Copy link
Contributor

We wanted to get some more eyes on this, but after talking with @d3viant0ne we agreed that it was OK to merge. Keep an eye out for a new minor version of the module tomorrow.

@the-spyke
Copy link

the-spyke commented Aug 8, 2017

Just updated my deps, and looks like this broke my app:

:8086//sockjs-node/iframe.html#vz40durl
Failed to load resource: the server responded with a status of 404 (Not Found)
:8086//sockjs-node/210/zkmtecqi/jsonp?c=_jp.akv1ra3:1
Uncaught SyntaxError: Unexpected token <
webpack-internal:///630:146
[WDS] Disconnected!

@shellscape
Copy link
Contributor

@the-spyke that's a bummer and I'm sorry that happened. But without for more info that's not very helpful. I'd suggest opening a new issue, following the issue template, and outlining more. Consider adding an example that would allow developers to reproduce the errors you're seeing.

@renchap
Copy link

renchap commented Aug 8, 2017

An issue about this has been opened: #1021

shellscape added a commit that referenced this pull request Aug 8, 2017
@shellscape shellscape mentioned this pull request Aug 8, 2017
shellscape added a commit that referenced this pull request Aug 8, 2017
* Revert "Sockjs prefix config (#911)"

This reverts commit ccd113a.

* Revert "2.7.0 (#1020)"

This reverts commit c8b9a0f.
@shellscape shellscape mentioned this pull request Oct 27, 2017
3 tasks
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.

None yet

7 participants