Revert "Remove __non_webpack_require__ workaround and split Node dependencies correctly (#48154)" #55229
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
the
area-signalr
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
3 tasks
mitchdenny
approved these changes
May 9, 2024
There was a problem hiding this comment.
I'd love for the SignalR package to be directly importable into a modern browser using the standard ES module support that is available. When I looked at this last time I think the issue was that some of the bundlers (can't remember if webpack was an issue) just didn't have good support for it yet.
|
/backport to main |
|
Started backporting to main: https://github.com/dotnet/aspnetcore/actions/runs/9069978442 |
|
@BrennanConroy backporting to main failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Revert "Remove __non_webpack_require__ workaround and split Node dependencies correctly (#48154)"
Using index info to reconstruct a base tree...
M src/SignalR/clients/ts/signalr/package.json
Falling back to patching base and 3-way merge...
Removing src/SignalR/clients/ts/signalr/src/DynamicImports.ts
Removing src/SignalR/clients/ts/signalr/src/DynamicImports.browser.ts
Auto-merging src/SignalR/clients/ts/signalr/package.json
CONFLICT (content): Merge conflict in src/SignalR/clients/ts/signalr/package.json
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Revert "Remove __non_webpack_require__ workaround and split Node dependencies correctly (#48154)"
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@BrennanConroy an error occurred while backporting to main, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 93520b6.
It caused more issues than it solved see #52082
We'll need to reopen #47674 and probably completely redo how we bundle the signalr package so it works better.
Revert "Remove non_webpack_require workaround and split Node dependencies correctly
Description
We made a change to fix some esbuild issues with the signalr npm package. Unfortunately, these changes had unintended side-effects to Angular apps.
Customer Impact
People using Angular 17 have build errors when using the
@microsoft/signalrpackage.Workarounds are given in #52082 (comment)
Regression?
Change in 8.0 to how we referenced external dependencies for node apps. Broke people using Angular 17 (which released after the change).
Risk
Completely reverting a change to how it was for multiple releases so there isn't any real risk. However, it is technically a breaking change when using esbuild. It's sad, but far more people are being affected by the current issue than were fixed by the change. A full fix would likely also be a breaking change as it likely requires reworking how we bundle the package which changes what files are available in the bundled package.
Verification
Packaging changes reviewed?