webpack / webpack

A bundler for javascript and friends. Packs many modules into a few bundled assets. Code Splitting allows for loading parts of the application on demand. Through "loaders", modules can be CommonJs, AMD, ES6 modules, CSS, Images, JSON, Coffeescript, LESS, ... and your custom stuff.
https://webpack.js.org
MIT License
64.78k stars 8.83k forks source link

globalObject: "this" work incorrect with ModuleFederation after 5.2.0 #12031

Open lomocc opened 3 years ago

lomocc commented 3 years ago

Bug report

What is the current behavior? the ModuleFederation crashed with output.globalObject: "this"

If the current behavior is a bug, please provide the steps to reproduce.

the output remoteEntry.js error:

// this is undefined
var chunkLoadingGlobal = this["webpackChunkreact_app_shared"] = this["webpackChunkreact_app_shared"] || [];

What is the expected behavior?

ModuleFederation should works correct with: output.globalObject: "this"

Other relevant information: webpack version: 5.2.0~5.5.1

lomocc commented 3 years ago

it works correct when set output.globalObject: "self"

alexander-akait commented 3 years ago

Can't reproduce, all works fine, please create minimum reproducible test repo and I will reopen an issue, both values works fine reproduced

alexander-akait commented 3 years ago

I think it is the limitation, when you set globalObject webpack attach all runtime to this object, but in some cases it is not safe to do it, so it is why we use self by default, if you use globalObject: 'something' you will get problem too, what is use case to use this?

lomocc commented 3 years ago
  1. create-react-app use globalObject: this for worker.
  2. it works well at 5.1.3(with arrowFunctions

so I think this is a bug.

alexander-akait commented 3 years ago

@lomocc No, it is not bug, it is limitation, globalObject: this will not work in workers, there is not this in web workers, you should use self

alexander-akait commented 3 years ago

it works well at 5.1.3(with arrowFunctions)

Yes, because arrow functions doesn't create context

sokra commented 3 years ago

It could be indeed a bug. Sounds like the bundle becomes strict mode only and this is undefined in strict mode.

As workaround avoid this as global object. Using the default should be fine instead.

webpack-bot commented 3 years ago

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

alexander-akait commented 3 years ago

bump

RReverser commented 3 years ago

I'm running into an issue where new Worker(new URL(...)) syntax used with a worker target that uses e.g.

this.onmessage = console.log;

silently produces a broken worker that contains only

console.log;

Is this the same issue or do I need to raise a separate one?

alexander-akait commented 3 years ago

@RReverser Can you provide more information? Need check

RReverser commented 3 years ago

Well... that's pretty much it. Put the code above:

this.onmessage = console.log;

in e.g. worker.js and then in main.mjs put

new Worker(new URL('./worker.js', import.meta.url));

And compile with Webpack with webpack ./main.mjs --mode=production.

The output for the worker contains just console.log;.

RReverser commented 3 years ago

Perhaps it's assuming that any Worker is ESM and hence strict by default, when it should be looking for new Worker(..., {type: 'module'}) to determine whether it's a script Worker, or a module Worker.

sokra commented 3 years ago

this in modules (e. g. CommonJs or ESM) is not the same as this in a script.

this.onmessage = console.log; this is equal to exports.onmessage = console.log; in CommonJs or undefined.onmessage = console.log; in ESM. So that won't work.

RReverser commented 3 years ago

@sokra I'm aware it's not the same. That's what I'm saying - that Webpack seems to assume that Worker is always a module, whereas by default in browsers it's a regular script and this is just a global object.

That is, if you try the code above in a browser, it works as expected, and this.onmessage will set the handler and log events.

RReverser commented 3 years ago

Perhaps it would be easiest for Webpack to just skip over workers without type: 'module' for now? Unless there's an easy way to tell it that others are regular scripts and should correspondingly have this == global object.

sokra commented 3 years ago

You could try new ProvidePlugin({ this: "global" }) maybe that works are workaround.

But it affects all modules...

Otherwise you can write your code as:

const asset = new URL("./worker.js", import.meta.url);
const worker = new Worker(asset);

This skips webpack processing for the worker file and treats it as normal asset.

RReverser commented 3 years ago

Yeah I guess that works as a workaround - in my case I went ahead and updated that legacy Worker that caused an issue to be strict mode compliant.

I just wanted to report this as it will probably catch others by surprise too, and takes a while to debug what's happening.

I think Webpack should either skip non-module usages of Workers, or provide correct environment to them, or at least warn on such usages - current behaviour quietly emits broken code, which seems like the worst of those options DX-wise.

Edge00 commented 3 years ago

My case: First i found complied code contains arrow fucntion. so i add target: ['web', 'es5'] to solve the problem. Then code became:

! function () {
  "use strict";
  /* `this` is undefined in strict mode */
  n = this.webpackChunkFoo = this.webpackChunkFoo || []
}

So i need to add output.globalObject: 'window' to solve this problem.

But it seems a little weird because once you set target: ['web', 'es5']. You need to configure output.globalObject: 'window' too. Which is not mentioned in documentation.

webpack-bot commented 3 years ago

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

vankop commented 2 years ago

@alexander-akait what should we do here? sounds like this is limitation to use globalObject: "this" ( in this case )


Mostly we use arrow functions where we can use them..

alexander-akait commented 2 years ago

@vankop I think we should not use arrow function in some cases, but yes, it can be tricky to analyze (maybe regexp or warning?)

vankop commented 2 years ago

I think we should not use arrow function in some cases

we use it only if environment supports it.

RReverser commented 2 years ago

Still an issue probably.