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

Issue with Worker & SSR (universal) #20877

Closed
1 of 15 tasks
shlomiassaf opened this issue May 20, 2021 · 3 comments · Fixed by #20948
Closed
1 of 15 tasks

Issue with Worker & SSR (universal) #20877

shlomiassaf opened this issue May 20, 2021 · 3 comments · Fixed by #20948

Comments

@shlomiassaf
Copy link

shlomiassaf commented May 20, 2021

🐞 Bug report

Command (mark with an x)

  • new
  • build
  • serve
  • test
  • e2e
  • generate
  • add
  • update
  • lint
  • extract-i18n
  • run
  • config
  • help
  • version
  • doc

Is this a regression?

Yes, worked in v11

Description

Application with a web worker requires 2 TS compilations, one for the app itself and one for the worker.
This is supported by angular via the webWorkerTsConfig build target configuration.

We exclude the web worker file from the client build and include it in the webWorkerTsConfig file.
This works fine since the IVY loader will try compiling it with both programs, the first emit wins.

With SSR server compilation this is not the case, we don't have a worker compilation, instead we need to handle the case
where there is no worker available in the environment and provide an alternative.

In V11 this worked fine, since we used webpack 4 and worker-loader. In server builds we just
don't include the worker-loader which means it will not detect the worker TS file module and will not
try to compile it.

Now, with the new Webpack 5 native worker support we no longer use worker-loader.

new Worker(new URL('../search.worker', import.meta.url), { name: 'searchWorker', type: 'module' });

Webpack will natively detect the search.worker.ts module and add it to the compilation, passing it to the IVY loader
which now has only 1 TS program configuration, which exclude the worker files so it will fail saying that the TS file is not part of the compiler configuration and we should add it to "include" or "files".

As a workaround, I managed to bypass this issue with file replacements and in server builds I just replace the file referencing the worker (new URL('...')) with a noop file that does not reference it.
This is good for some scenarios but other might not fit this.

I guess one can also go back to the worker-loader method and change the new URL syntax to a literal string.

🌍 Your Environment


@angular-devkit/architect       0.1200.0
@angular-devkit/build-angular   12.0.0
@angular-devkit/core            12.0.0
@angular-devkit/schematics      12.0.0
@angular/flex-layout            12.0.0-beta.34
@nguniversal/common             12.0.0
@nguniversal/express-engine     12.0.0
@schematics/angular             12.0.1
ng-packagr                      12.0.0
rxjs                            6.6.3
typescript                      4.2.4

Anything else relevant?

@alan-agius4
Copy link
Collaborator

Having a file in the a server compilation that depends on Browsers APIs seems wrong to me because Node.JS worker API is not fully interchangeable with that of the Browser, even how they are refereced where in the browser the Worker is available in the global context, while on the server you need to import it from the worker_threads module.

The fact that the build previously succeeded seems like a problem, since that might have caused incorrect behaviour if you were dependent on the worker to render the page.

I am also surprised to why this didn't previously crash the server during runtime with an error like ReferenceError: Worker is not defined since on Node.JS Worker doesn't exist in global context.

@shlomiassaf
Copy link
Author

shlomiassaf commented May 23, 2021

Hi @alan-agius4, thanks for taking the time to explain.

From the docs:
image

The reason there will not be a reference error is the safe guard checking.

if (typeof Worker !== 'undefined') {
  this.adapter = new WorkerSearchAdapter();
} else {
  this.adapter = new WindowSearchAdapter();
}

It seems, AFAIK, that there's no real solution here.
In any scenario you have to port to the proper module, which typeof Worker !== 'undefined' seems to handle fine.

The problem is in the module that holds WorkerSearchAdapter.
It will contain the line of code to load the worker:

new Worker(new URL('../search.worker', import.meta.url), { name: 'searchWorker', type: 'module' });

In older webpack, this was handled by worker-loader which, if not inclued in the webpack loaders will just skip this line without processing '../search.worker', now in webpack 5 it does, and since it's a server build we get that type issues cause the JS spec's lib definitions for browser collide with worker library types.

I worked around this one using the fileReplacements feature through angular.json. But again, it's cumbersome and strange since it bind some deep code with specific settings and it will not suite all scenarios.

Since I do have safe runtime type protection I do need webpack to skip this one. The error is thrown because webpack will pass this file to the loaders while the file is EXCLUDED from the loaders.


The CLI must be smart here and detect that this file was explicitly excluded in TS configuration so webpack should not really process it.
I didn't check the webpack configuration which instruct what files to pass to the loaders but it seems that other excluded files are NOT passed to the loaders, so this is an exception which webpack bypasses due to that native implementation to Worker.

It might be a bug in webpack, or how webpack detect TS files... I don't know these internal between CLI and webpack

@ngbot ngbot bot modified the milestone: needsTriage May 23, 2021
@ngbot ngbot bot modified the milestones: needsTriage, Backlog May 23, 2021
@alan-agius4 alan-agius4 self-assigned this May 23, 2021
filipesilva pushed a commit that referenced this issue May 31, 2021
…ver builds

Web-workers are not supported on the server and therefore we should not try include them in server builds.

Closes #20877
filipesilva pushed a commit that referenced this issue May 31, 2021
…ver builds

Web-workers are not supported on the server and therefore we should not try include them in server builds.

Closes #20877

(cherry picked from commit 77f81dc)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants