-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
monaco ESM build contains an untranspiled require #759
Comments
That is the code path for the AMD case. It could be reached if a language tries to create a web worker and the correct worker file path is not returned. i.e. a missing or incomplete:
I am not sure what we should do, as this is ... a configuration error... |
Maybe the correct solution here would be to fall back to a dynamic |
Yes, that is typically the approach we have taken. The problem is that code should not execute, and it executes due to a configuration error. The ESM case should fall in this and not fall back to trying to dynamically load the specializing module. i.e. if it would be a dynamic |
If I understand this correctly, it might take a little rewriting, but I think it would be achievable via webpack contexts – specifically by creating a ContextReplacementPlugin in the project's webpack config, where you would explicitly specify paths for all the worker fallbacks that could be dynamically imported at runtime. That way, when webpack encounters the dynamic I'm not an expert on this, and the documentation is a bit vague on how this operates, but maybe a webpack pro like @TheLarkInn could chime in on whether this is a valid approach? |
@timkendrick Thanks! But I don't believe there is anything to change on our side. The editor will at one point invoke ... IMHO the exception occurs because the integrator did not implement |
UPDATE: I've since got fallbacks working via webpack contexts (see my next comment) @alexandrudima I see your point that the worker should always be present and correct (at least as much as a dynamic import would be), but it feels to me like good practice to provide a fallback in case the worker fails for whatever reason. I've been doing some investigation and it's definitely still possible to use the existing fallback mechanism under a webpack environment (see my fork of the monaco webpack sample which fixes this issue). Looking at the vscode source, I can see why a plain context replacement wouldn't work on its own: the fallback modules are loaded via a call to Now at this point we have a couple of different options. The cleanest one in my opinion would be to change the I'll try to investigate this further if I get a chance, but in the meantime, I got a version working that essentially does the same thing manually via a babel AST transform (see the webpack.config.js in my fork of the sample project). This will use the main-thread fallback versions of the worker scripts if the worker fails to load for some reason, as intended in the original AMD version. It works by applying a babel transform to the Example source: self.require([moduleId], function (foreignModule) {
... Babel-transformed source: (function (_moduleIds, _callback) {
var _paths = {
'vs/language/css/cssWorker': require('/Users/tim/Sites/monaco-editor-samples/browser-esm-webpack/node_modules/monaco-editor/esm/vs/language/css/cssWorker.js'),
'vs/language/html/htmlWorker': require('/Users/tim/Sites/monaco-editor-samples/browser-esm-webpack/node_modules/monaco-editor/esm/vs/language/html/htmlWorker.js'),
'vs/language/json/jsonWorker': require('/Users/tim/Sites/monaco-editor-samples/browser-esm-webpack/node_modules/monaco-editor/esm/vs/language/json/jsonWorker.js'),
'vs/language/typescript/tsWorker': require('/Users/tim/Sites/monaco-editor-samples/browser-esm-webpack/node_modules/monaco-editor/esm/vs/language/ts/tsWorker.js')
};
return _callback.apply(undefined, _moduleIds.map(function (_moduleId) {
if (!(_moduleId in _paths)) throw new Error('Module not found: ' + _moduleId);
return _paths[_moduleId];
}));
})([moduleId], function (foreignModule) {
... Webpack output: (function (_moduleIds, _callback) {
var _paths = {
'vs/language/css/cssWorker': __webpack_require__(63),
'vs/language/html/htmlWorker': __webpack_require__(62),
'vs/language/json/jsonWorker': __webpack_require__(61),
'vs/language/typescript/tsWorker': __webpack_require__(60)
};
return _callback.apply(undefined, _moduleIds.map(function (_moduleId) {
if (!(_moduleId in _paths)) throw new Error('Module not found: ' + _moduleId);
return _paths[_moduleId];
}));
})([moduleId], function (foreignModule) {
... With a little more effort, I'm sure the babel transform could output asynchronous Does this help at all? |
@alexandrudima I configured the environment correctly, but I provided paths that were from a different origin because I forgot about the same origin restriction. The web workers then failed to load, which caused monaco to go down the fallback code path and trigger this error. Here’s a screenshot: In my case, I need to come up with a way to serve web workers from our CDN, but until then, the non-webworker case is very valid for me. |
Ok, so I've figured out how to get this working with the @meyer, does the sample at https://github.com/timkendrick/monaco-editor-samples/blob/feature/worker-context-fallbacks/browser-esm-webpack/webpack.config.js fix the issue for you? (you'll need to install the The babel loader is now only needed to transform the While this works, the configuration for the I'm imagining something like this in the user's webpack config: const webpack = require('webpack');
const MonacoWebpackPlugin = require('monaco-editor/webpack');
module.exports = {
...,
plugins: [
new MonacoWebpackPlugin(webpack), // creates a preconfigured webpack.ContextReplacementPlugin
]
}; …or if they wanted to provide a custom set of workers: plugins: [
new MonacoWebpackPlugin(webpack, {
'vs/language/xyz/xyzWorker': 'monaco-editor/esm/vs/language/xyz/xyzWorker',
})
] |
@timkendrick that does the trick! Thanks! |
Running along with the ideas in my previous comment, I've built an experiment where all the Monaco webpack configuration is bundled up as a single webpack plugin that the user includes in their webpack config. It's definitely much more straightforward than before: there's no need for all the It's also nice that the user doesn't need to know anything about the internals of the To include the full editor with all the languages etc you just need: webpack.config.js: const webpack = require('webpack');
const MonacoWebpackPlugin = require('monaco-editor/webpack');
module.exports = {
entry: './index.js',
output: {
path: path.resolve(__dirname, 'dist'),
filename: 'app.js'
},
plugins: [
new MonacoWebpackPlugin(webpack)
]
}; index.js: import * as monaco from 'monaco-editor';
monaco.editor.create(document.getElementById('container'), {
value: 'console.log("Hello, world")',
language: 'javascript'
}); …or to include a subset of the languages/features, you pass the configuration into the webpack plugin: webpack.config.js: const webpack = require('webpack');
const MonacoWebpackPlugin = require('monaco-editor/webpack');
module.exports = {
entry: './index.js',
output: {
path: path.resolve(__dirname, 'dist'),
filename: 'app.js'
},
plugins: [
new MonacoWebpackPlugin(webpack, {
languages: ['python'],
features: ['coreCommands', 'findController']
})
]
}; index.js: import * as monaco from 'monaco-editor/esm/vs/editor/editor.api.js';
monaco.editor.create(document.getElementById('container'), {
value: 'print("Hello, world")',
language: 'python'
}); Things like language aliases and feature names could probably do with a bit of tweaking, but the mechanism seems to work reliably, as well as handling worker fallbacks gracefully. You can see the source for the plugin itself here, and see examples for the two different setups here and here. I've tested it by npm installing my modified Let me know if this is helpful / if you have any questions! |
Even if the errors are addressed, this is still IMHO a mis-configured webpage. Web workers are not starting up because the JS is served from a different domain than the HTML. And then the heavy-lifting code (which creates ASTs and other heavy stuff) runs in the UI thread. This can lead to UI freezes and a horrible a user experience! To reproduce, try to paste a 2MB TypeScript file in the editor you are hosting (you can use The correct thing to do is to configure the web workers correctly: e.g. Assuming the HTML lives at www.mydomain.com: self.MonacoEnvironment = {
getWorkerUrl: function (moduleId, label) {
if (label === 'json') {
return 'https://www.mydomain.com/json.worker.redirect.js';
}
if (label === 'css') {
return 'https://www.mydomain.com//css.worker.redirect.js';
}
if (label === 'html') {
return 'https://www.mydomain.com//html.worker.redirect.js';
}
if (label === 'typescript' || label === 'javascript') {
return 'https://www.mydomain.com//ts.worker.redirect.js';
}
return 'https://www.mydomain.com//editor.worker.redirect.js';
}
} Then you must create/host 5 worker redirect files on www.mydomain.com:
importScripts('www.mycdn.com/json.worker.bundle.js'); ... You can verify web workers are running by not seeing any warnings in the console or by looking in the dev tools for the Threads tab: |
Hi @alexandrudima – thanks for looking into this. I realise I've been explaining myself badly and writing a lot, so I'll try to summarise:
I agree (which is partly why I was proposing a plugin that minimises the chance of misconfiguration). However it's often seen as good practice to provide a fallback for the possibility that things fail due to network errors etc. That's presumably why this fallback was written in the first place. Two questions:
Agreed again – however this is a fallback, so presumably the alternative is having no language services at all, which I would say is worse. Of course, 99% of the time the worker script will be loaded correctly and the fallback code path won't be hit. The fallback is there for the 1% of cases where the worker script doesn't load because they're using IE9, or because their corporate proxy doesn't allow certain types of request. I'd understand if you decided to just take the fallback out completely (although I think this would be a bad idea), however what I don't understand is having a broken fallback that crashes the user's app with a confusing error message when the worker script fails to load. |
@alexandrudima yep, i understand the fallback path is not ideal. I read and understand the issues. At the moment, the web app I work on is not configured to serve assets from the same domain as the app. Until this issue has been fixed on our end, I will need to use the web worker fallback to test monaco. As I understand it, the fallback path is a legitimate one and should be available to everyone, since the edge case it’s designed to handle is legitimate and has nothing to do with how monaco is bundled. |
@meyer I agree with @alexandrudima on this one, that the fallback should ideally be used only as a fallback and not as the primary means of loading the worker scripts. I also understand how there are some scenarios (e.g. yours), that you just don't have the liberty of being able to load external resources. My suggestion is to get creative with webpack and write some custom loaders that compile the worker scripts and transform the compiled source into blob URLs. These blob URLs can then be returned from the Obviously if you go down this route you'll end up with an enormous bundle that takes a couple of seconds for the browser to parse, but that's probably unavoidable. I've written some working examples for this approach (see links at the end of this comment), but here's the step by step instructions: First you'll need to update your index.js: self.MonacoEnvironment = {
getWorkerUrl(moduleId, label) {
switch (label) {
case 'json': return require('blob-url-loader?type=application/javascript!compile-loader?target=worker&emit=false!monaco-editor/esm/vs/language/json/json.worker');
case 'css': return require('blob-url-loader?type=application/javascript!compile-loader?target=worker&emit=false!monaco-editor/esm/vs/language/css/css.worker');
case 'html': return require('blob-url-loader?type=application/javascript!compile-loader?target=worker&emit=false!monaco-editor/esm/vs/language/html/html.worker');
case 'typescript':
case 'javascript': return require('blob-url-loader?type=application/javascript!compile-loader?target=worker&emit=false!monaco-editor/esm/vs/language/typescript/ts.worker');
default:
return require('blob-url-loader?type=application/javascript!compile-loader?target=worker&emit=false!monaco-editor/esm/vs/editor/editor.worker');
}
},
};
…then in your webpack config you'll need to add a webpack.config.js: module.exports = {
...
resolveLoader: {
alias: {
'blob-url-loader': require.resolve('./loaders/blobUrl'),
'compile-url-loader': require.resolve('./loaders/compile'),
},
},
}; …then in the I've put together some samples of this in action: see the batteries-included version and the minimal version which both seem to work fine. Hope that helps! |
@timkendrick omg thank you. i owe you bigtime. |
Glad that helped! Now we just need someone to publish this as |
BTW in case anyone's following along at home, I've just added another webpack example that outputs a standalone library version as a single
…so if anyone wants to publish a standalone library build as an npm module, it should be as easy as:
|
@timkendrick that bundle is, unfortunately, rather misleading, because it is not minimal. your composition (browser-esm-webpack-bundled-library): You can bet you have typescriptServices.js in ts.worker.js, too, because ts.worker.js is just that big. The following is the composition of "browser-esm-webpack" (also a sample of the sample repository) Here you can see 2x typescriptServices.js (and more dupes). Your "minimal" bundle is the same, except that the bundle analyser cannot look into your ts.worker.js because of the inlined stuff, I suppose :) Sad but true. Also, won't be able to be fixed that quickly, see #776 . Webpack seems to have problems with code splitting web workers. If you don't want dupes, sadly the AMD version is the only route atm. At least no typescriptServices.js dupe there :) edit: forgot: https://github.com/webpack-contrib/webpack-bundle-analyzer |
@neoncom Thanks for bringing this up – you're right, perhaps 'minimal' was the wrong name for that version… 😉 To me it seems like there are probably two kinds of redundancy going on here:
|
@timkendrick https://github.com/timkendrick/monaco-editor-samples/blob/feature/worker-context-fallbacks/browser-esm-webpack/webpack.config.js does not work, i get (in editor.worker.bundle.js:163):
this code is making problems:
do you have an idea what that could be? (i have codesplitting disabled) |
@neoncom sorry, just realised I only tested the fallback case, not the actual happy path :) This is due to the editor samples using the same webpack settings to bundle both the web worker scripts and the main app bundle. This means the web worker scripts were being webpacked using the default I've pushed an update to my branch that fixes this. |
@alexandrudima a bit confused on |
I tried setting I also tried the webpack plugin and still see the same issue. Debuggin is incredibly annoying because my whole browser starts lagging heavily because of the fallback to load into the UI thread. I wish the fallback could be disabled. |
Because it is possible to trigger the creation of a web worker without creating an editor. e.g. We have a lot of self-contained working samples at https://github.com/Microsoft/monaco-editor-samples. Are you using |
I tracked it down - the error that causes the WebWorker creation to fail is /******/ var jsonpArray = window["webpackJsonp"] = window["webpackJsonp"] || []; The fix is to set |
@felixfbecker When I run the example at https://github.com/Microsoft/monaco-editor-samples/tree/master/browser-esm-webpack , I cannot find |
The only difference I see is that I am using webpack 4.5.0, while the example uses 4.1.1 |
@alexandrudima The issue is only for 0.12.0 AFAIK for some reason. The sample repo for esm still uses 0.11.0 |
I have updated the samples at https://github.com/Microsoft/monaco-editor-samples and at https://github.com/Microsoft/monaco-editor/blob/master/docs/integrate-esm.md to use |
edit: any reason you don't pre-build these files and we just serve them as is? |
They are prebuilt in AMD form under the /amd/ folder. Webpack can also load them in AMD form, it is just that webpack must be taught to not attempt to package them again and rather do an xcopy when packing. |
To avoid further derails of this issue, I suggest we close it for now, as the original issue the OP reported is fixed. Let's use new issues when new problems are found. |
@alexandrudima @timkendrick |
Hi there. I was delighted to see that you all are now shipping an ESM build of
monaco-editor
(#18 (comment))! I took it for a spin but encountered what seems to be a bug.I’ve been using the example monaco + webpack project as reference. The primary editor code loads without issue, but if for some reason web workers fail to load, the web worker fallback throws the following error due to an untranspiled require:
monaco-editor version: 0.11.0
Browser: Chrome 64
OS: OSX
Steps or JS usage snippet reproducing the issue:
monaco-editor
0.11.0Let me know if there are any other details you’d like me to provide. Thanks!
The text was updated successfully, but these errors were encountered: