Fix broken web workers when using CDN

What does this MR do?

In order to work around web workers same-origin policy, we must override the CDN settings on pages which utilize web workers. The is done by overriding __webpack_public_path__ which sets the base path for all dynamically loaded javascript assets at runtime.

The monaco-editor-webpack-plugin has a bug in which it tries to reference this path in the global scope (window.__webpack_public_path__) instead of as a free variable (__webpack_public_path__) – see https://github.com/Microsoft/monaco-editor-webpack-plugin/pull/63 . In !26015 (merged) we edited our workaround to set the globally scoped variable to fix the IDE, but this in turn broke our non-monaco web workers which were relying on the old behavior.

This MR reverts the changes in !26015 (merged) by removing the deleted lines and adding a note above our workaround line for the monaco plugin so that we can remove it once the bug is fixed upstream.

Original Description:

__webpack_public_path__ must be a "free variable" according to the documentation. It does not exist on the global scope, and setting window.__webpack_public_path__ will have no effect.

Effectively this change disabled the setting of __webpack_public_path__ prior to loading the IDE assets. If doing so managed to fix the issue, we ought to be explicit about it and just remove the line altogether. However I believe that will introduce other problems, as the reason it was here in the first place was to ensure the web worker scripts bypass CDN settings since they do not work cross-origin. I suggest reverting for now and re-forming a new solution.

/cc @iamphill @filipa

What are the relevant issue numbers?

Re-opens #50451 (closed)

Closes: gitlab-org/gitlab-ce#60217

added frontend maintenancerefactor typebug webpack labels

@iamphill can you review this? happy to discuss a bit more first if you'd like

assigned to @iamphill

I'm wondering if there is a good way to formulate a regression test for things like CDN settings for cross-origin issues because it would be easy to not notice when something like this breaks until it gets deployed to gitlab.com.

Like I replied in the original merge request, I tested with a relative root URL & the Web IDE correctly loaded the editor.worker.js. It also looks like the monaco-loader looks for the webpack public path variable on window (https://github.com/Microsoft/monaco-editor-webpack-plugin/blob/master/index.js#L120)

We could write our own MonacoEnvironment, but we'd just be writing what already exists

assigned to @mikegreiling

The Monaco we pack loader uses it when setting worker URLs https://github.com/Microsoft/monaco-editor-webpack-plugin/blob/master/index.js#L120

🤨 wow that's odd. Good find! Monaco is using __webpack_public_path__ in a non-standard way. Ordinarily this would not work if it is intending to access webpack's internal publicPath setting... I wonder if this was intentional.

Well if this does work I'm fine with keeping it, but I'd like to note this peculiarity with a descriptive comment and maybe file an issue upstream. I'm not sure we want to rely on what might be a bug in Monaco's implementation.

I'd also like to test out the IDE via a CDN just to make absolutely sure it is working.

It looks like there's already an issue for this: https://github.com/Microsoft/monaco-editor-webpack-plugin/issues/7#issuecomment-447224683

Perhaps a good solution would be to set both __webpack_public_path__ and window.__webpack_public_path__ here, with a note to remove the latter once this bug is fixed upstream.

We could write our own MonacoEnvironment, but we'd just be writing what already exists

This wouldn't be such a bad idea actually. When we set the webpack public path, it actually changes it for all dynamically loaded chunks, meaning it effectively disables the CDN. However this is a bit overkill, because we could still use the CDN for non-worker scripts. If we modified getWorkerUrl only, we could target only the scripts that need this.

mentioned in merge request !26015 (merged)

mentioned in issue #60217 (closed)

Perhaps a good solution would be to set both __webpack_public_path__ and window.__webpack_public_path__ here, with a note to remove the latter once this bug is fixed upstream.

I was going to suggest the same.

changed milestone to %11.10

added 1 deleted label

added 1608 commits

• 61b9ee8f...109f6122 - 1606 commits from branch master
• 596d5fa2 - Revert "Merge branch 'fix-ide-web-worker-relative-url' into 'master'"
• cb55c32a - Add CHANGELOG.md entry

Compare with previous version

changed the description

@stanhu could you test and review this MR?

assigned to @stanhu

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.

Category	Reviewer	Maintainer
frontend	Martin Wortschack (@wortschi)	Filipa Lacerda (@filipa)

Generated by Danger

added backstage [DEPRECATED] label

resolved all discussions

I tried to test this by changing config.action_controller.asset_host = 'localhost.me:3000'. While the CSS loaded fine, it looks like Webpack was confused by host vs. URL:

`

I'm inclined just merge this and get a build running on staging ASAP.

Ok, this is the right config:

config.action_controller.asset_host = 'http://localhost.me:3000'

I also had to disable inlining:

diff --git a/config/webpack.config.js b/config/webpack.config.js
index 19b48845305..41ca722844a 100644
--- a/config/webpack.config.js
+++ b/config/webpack.config.js
@@ -172,7 +172,7 @@ module.exports = {
             loader: 'worker-loader',
             options: {
               name: '[name].[hash:8].worker.js',
-              inline: IS_DEV_SERVER,
+              inline: false,
             },
           },
           'babel-loader',

Without this MR, I replicated the error on staging:

With this MR in my dev environment, I got a slightly different URL:

Is this just an issue with a local dev environment?

resolved all discussions

approved this merge request

enabled an automatic merge when the pipeline for 596d5fa2 succeeds

@mikegreiling Can you update the description of the merge request to better reflect this latest change?

resolved all discussions

changed the description

canceled the automatic merge

merged

mentioned in commit 5cc1d2f1

changed title from Revert "Merge branch 'fix-ide-web-worker-relative-url' into 'master'" to Fix broken web workers when using CDN

Automatically picked into https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/27157, will merge into 11-10-stable ready for 11.10.0-rc4.

removed 1 deleted label

mentioned in commit 9f38d3db

mentioned in merge request !27157 (merged)

mentioned in issue gitlab-org/release/tasks#738 (closed)

mentioned in issue gitlab-org/release/tasks#778 (closed)