Skip to content

Fix SSR issues for SignalR: require is not defined #19832

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
Merged
merged 3 commits into from
Mar 23, 2020
Merged

Fix SSR issues for SignalR: require is not defined #19832

merged 3 commits into from
Mar 23, 2020

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented Mar 13, 2020 •
edited
Loading

Fix SSR issues for SignalR: require is not defined.

  • Moved requireFunc to constructor so that import SignalR module won't break SSR.
  • Removed a redundant tailing blank-space for ConnectionState.Connecting.

Addresses #10400.

@ghost ghost added the area-signalr Includes: SignalR clients and servers label Mar 13, 2020
@BrennanConroy
Copy link
Member

Thanks for the PR!

You're a little too early because we're currently getting rid of request in #19708. But your change could apply to the new code once it's merged.

I'll try to merge the change later today so you can rebase and apply this fix to the new code.

@BrennanConroy
Copy link
Member

Ok, change has been merged. Please rebase and update!

@hez2010
Copy link
Contributor Author

hez2010 commented Mar 14, 2020 •
edited
Loading

@BrennanConroy I've rebased my branch and applied fixes also to the new FetchHttpClient.

@hez2010 hez2010 changed the title Fix SSR issues for SignalR: Cannot find module 'request' Fix SSR issues for SignalR: require is not defined Mar 14, 2020
BrennanConroy
BrennanConroy reviewed Mar 16, 2020
View reviewed changes
src/SignalR/clients/ts/signalr/src/FetchHttpClient.ts
// Node needs EventListener methods on AbortController which our custom polyfill doesn't provide
this.abortControllerType = requireFunc("abort-controller");
} else {
this.fetchType = fetch.bind(self);
Copy link
Member

Choose a reason for hiding this comment

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

Why call bind?

Copy link
Contributor Author

@hez2010 hez2010 Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Without calling bind, it will cause error if you try to invoke fetch:

Network error: TypeError: Failed to execute 'fetch' on 'Window': Illegal invocation

It seems to be a bug coming from fetch, where a binding is set incorrectly. Calling fetch.bind(self) can resolve the problem, which will bind default fetch to "global" scope.

@hez2010
Copy link
Contributor Author

hez2010 commented Mar 21, 2020

Any updates on this PR?

BrennanConroy
BrennanConroy approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Any updates on this PR?

Sorry, was gone for a long weekend. Looks good!

@BrennanConroy BrennanConroy merged commit 56d50e6 into dotnet:master Mar 23, 2020
@BrennanConroy
Copy link
Member

Thanks for the PR @hez2010 !

@BrennanConroy BrennanConroy added this to the 5.0.0-preview3 milestone Mar 23, 2020
@BrennanConroy BrennanConroy mentioned this pull request Mar 23, 2020
Closed
@hez2010 hez2010 mentioned this pull request May 2, 2020
Closed
@hez2010
Copy link
Contributor Author

hez2010 commented May 3, 2020

Can I backport this fix to 3.1 release (see #21059 (comment))?

Since it introduce no breaking changes.

@sevillal sevillal mentioned this pull request Aug 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@analogrelay analogrelay Awaiting requested review from analogrelay

@halter73 halter73 Awaiting requested review from halter73

Assignees
No one assigned
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Milestone
5.0.0-preview3
Development

Successfully merging this pull request may close these issues.
2 participants
@hez2010 @BrennanConroy