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

Add webpack loader plugin to simplify ESM usage #767

Merged
merged 6 commits into from
Mar 20, 2018

Conversation

timkendrick
Copy link
Contributor

@timkendrick timkendrick commented Mar 16, 2018

The recent ESM build works brilliantly, and it's really exciting to finally have the monaco editor as a first-class citizen of the webpack ecosystem.

Unfortunately, the user needs to make several non-intuitive additions to their webpack config and source code before they can use it:

  1. Add additional entry points for each of the required worker scrips
  2. Manually create a MonacoEnvironment that references the output paths provided for these entry points
  3. Add a webpack.IgnorePlugin() that references an internal file within monaco-editor package
  4. (optional) Manually import specific feature modules from deep within the monaco-editor package if the user wants to customise the editor build

None of these steps is obvious without deep knowledge of how monaco-editor code is structured internally, increasing the potential for various unexpected gotchas along the way.

This PR demonstrates a potential technique to simplify importing the Monaco Editor into a webpack project by exporting a webpack plugin from the monaco-editor package. It allows the user to import the editor with just a single addition to their webpack config, and automatically sets up the global environment, compiles additional worker files etc:

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'
});

For a more custom configuration, the user can specify which languages/features they want, as well as the worker scripts output directory:

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'],
      output: './workers/monaco'
    })
  ]
};

index.js:

import * as monaco from 'monaco-editor/esm/vs/editor/editor.api';

monaco.editor.create(document.getElementById('container'), {
	value: 'print("Hello, world")',
	language: 'python'
});

Points for improvement/discussion:

  • The monaco-editor vs monaco-editor/esm/vs/editor/editor.apidistinction is a bit confusing: if the primary target is webpack users then you could just set the main monaco-editor export to the editor.api.js version (rather than the current editor.main.js version) and let the plugin handle adding all the features. If you prefer to keep monaco-editor exporting the whole batteries-included editor, you could at least simplify the paths into say monaco-editor vs monaco-editor/custom or something.
  • The plugin options are currently not validated (webpack and its libraries typically use ajv for options schema validation)
  • The features could better named in webpack/features.js – for the time being I just took the basename of each file but a lot of them include Controller or whatever, and aren't great names for API options
  • I haven't updated the readme to include plugin usage instructions
  • Worker fallbacks are included as normal webpack imports – under default webpack settings this will generate separate files that are loaded in on-demand via JSONP, which is probably fine, however maybe import() expressions would be better from a code-splitting perspective
  • The plugin uses various of the inbuilt webpack plugins, so it needs a reference to the webpack library. I didn't want to introduce an additional "webpack" dependency to monaco-editor, so I've gone with passing a webpack instance into the plugin constructor instead. While this is arguably more flexible, it is susceptible to breaking changes in the webpack library. This could be worked around by adding webpack as a peerDependency of the monaco-editor package, or by extracting the plugin out into its own separate package entirely, which would have its own peerDependency on "webpack"
  • There's currently a babel transform that replaces the self.require() call in editorSimpleWorker.js with a global require() call. The babel transform could be removed if the vscode source were changed.

This doesn't impact how the package currently functions of course: the plugin is an optional add-on whose main task is to autogenerate webpack configuration rather than the user doing it manually. This means that if e.g. the user doesn't use webpack, they can just not use the plugin and they can continue as normal.

I realise this is quite a big addition, so if you guys want to re-implement this yourselves or just trash it or whatever that's fine, I just wanted to propose it as a potential avenue to explore. I'm eager to help make this library as easy as possible for users to consume in their projects, and I feel like something like this would be the missing piece of the puzzle. Let me know if there's anything I can do to help!

@msftclas
Copy link

msftclas commented Mar 16, 2018

CLA assistant check
All CLA requirements met.

@alexdima
Copy link
Member

This sounds absolutely great!

It will just take me some time to digest & understand all of this. ❤️.

@timkendrick
Copy link
Contributor Author

No problem, thanks for looking into it - I bashed out the code without commenting it up etc so it might be a bit confusing to read, especially with all the webpack hoops you have to jump through to make everything work.

There are probably a lot of things that need explaining, so let me know if there’s stuff that doesn’t make sense and I’ll try to be as responsive as possible!

@alexdima
Copy link
Member

@timkendrick

I am trying to remove the need for babel-loader by changing self.require to require in editorSimpleWorker.


But I am running into a problem while trying out the plugin:

const path = require('path');
const webpack = require('webpack');
const MonacoWebpackPlugin = require('monaco-editor/webpack');

module.exports = {
  entry: './index2.js',
  output: {
    path: path.resolve(__dirname, 'dist'),
    filename: 'app.js'
  },
  mode: 'development',
  plugins: [
    new MonacoWebpackPlugin(webpack)
  ],
  module: {
        rules: [{
            test: /\.css$/,
            use: ['style-loader', 'css-loader']
        }]
    },
};

I am always getting:

ERROR in chunk 0
0.app.js
Conflict: Multiple assets emit to the same filename 0.app.js

I see 0.app.js mentioned lower in the output, here:

Child vs/language/typescript/tsWorker:
                   Asset      Size  Chunks                    Chunk Names
    typescript.worker.js  7.86 MiB    main  [emitted]  [big]  main
                0.app.js  1.72 MiB       0  [emitted]  [big]
    Entrypoint main = typescript.worker.js
    [./node_modules/monaco-editor/esm/vs/editor/common/services sync recursive] ./node_modules/monaco-editor/esm/vs/editor/common/services sync 290 bytes {0} [built]
    [./node_modules/webpack/buildin/global.js] (webpack)/buildin/global.js 509 bytes {main} [built]
    [./node_modules/webpack/buildin/harmony-module.js] (webpack)/buildin/harmony-module.js 597 bytes {main} [built]
        + 108 hidden modules

and I believe that should not be the case, i.e. tsWorker should result in a single bundle file.

This might be a direct result of my adding:

  module: {
        rules: [{
            test: /\.css$/,
            use: ['style-loader', 'css-loader']
        }]
    },

to your example, but if I don't do that I am getting a lot of errors about the .css files.

Anyhow, what would be a complete working webpack.config.js such that I can try this out and not get any errors (either about CSS or about 0.app.js)?

@timkendrick
Copy link
Contributor Author

timkendrick commented Mar 19, 2018 via email

@alexdima
Copy link
Member

Thanks for the fast reply, even when trying out with your example, I still get that error. It is simply buried in the middle of the output, so I suppose it is easy to miss...

I managed to avoid the error by changing AddWorkerEntryPointPlugin.js and adding chunkFilename: 'workerchunk.[id].js', to the output options. This makes it that the extra code from tsWorker goes to a file called workerchunk.0.js and does not collide with the other 0.app.bundle.

I'll proceed to try to eliminate the need for babel rewriting.

@alexdima
Copy link
Member

On second thought, that is not a very good idea. Adding chunkFilename to the outputOptions in AddWorkerEntryPointPlugin.js does avoid the error, but makes the dist folder 60MB.

@timkendrick The curious thing is that everything works fine, despite of the error, so I'm not sure how to proceed. Is this normal? Should we just ignore the error as everything works fine?

@timkendrick
Copy link
Contributor Author

timkendrick commented Mar 19, 2018

Hmm, strange – I'm not seeing these duplicate entry points. When I run the browser-esm-webpack-plugin sample I see this:

Child vs/language/typescript/tsWorker:
                   Asset      Size  Chunks                    Chunk Names
    typescript.worker.js  7.86 MiB    main  [emitted]  [big]  main
    Entrypoint main = typescript.worker.js
    [../../vscode-monaco-editor/release/esm/vs/base/common/lifecycle.js] /Users/tim/Sites/vscode-monaco-editor/release/esm/vs/base/common/lifecycle.js 2.98 KiB {main} [built]
    [../../vscode-monaco-editor/release/esm/vs/base/common/platform.js] /Users/tim/Sites/vscode-monaco-editor/release/esm/vs/base/common/platform.js 3.21 KiB {main} [built]
    ...

Are you sure you're using the exact same webpack config as in the sample? You've not got e.g. multiple entry points, or [name] in output filename or something?

@alexdima
Copy link
Member

@timkendrick Yeah, that is really bizzare.

Here is what I'm doing:

  1. in monaco-editor, I am on the branch of this PR.
  2. in monaco-editor, I run npm install ., to be sure that the latest monaco-editor-core comes in.
  3. in monaco-editor, I run npm run release, to prepare the release folder
  4. I have https://github.com/alexandrudima/monaco-editor-webpack-plugin-test checked out. (I just double checked, it is the same as your example).
  5. in this folder, I run npm install .
  6. in this folder, I replace the node_modules/monaco-editor with the monaco-editor/release folder by deleting and copying (i.e. as if the monaco-editor was published to npm, I'm not even using links or something...)
  7. in this folder, I run ./node_modules/.bin/webpack:
...
Child vs/language/typescript/tsWorker:
                   Asset      Size  Chunks                    Chunk Names
    typescript.worker.js  7.86 MiB    main  [emitted]  [big]  main
         0.app.bundle.js  1.72 MiB       0  [emitted]  [big]
    Entrypoint main = typescript.worker.js
    [./node_modules/monaco-editor/esm/vs/editor/common/services sync recursive] ./node_modules/monaco-editor/esm/vs/editor/common/services sync 290 bytes {0} [built]
    [./node_modules/webpack/buildin/global.js] (webpack)/buildin/global.js 509 bytes {main} [built]
    [./node_modules/webpack/buildin/harmony-module.js] (webpack)/buildin/harmony-module.js 597 bytes {main} [built]
        + 108 hidden modules

@timkendrick
Copy link
Contributor Author

timkendrick commented Mar 19, 2018

Thanks a lot for the detailed repro instructions, that really helped! Sorry, I skim-read your earlier message while at work and didn't realise you had already replaced the self.require(). I had assumed this would just work but hadn't actually tested it, sorry about that.

I've fixed the problem: it seems to be due to the behavior of webpack's (completely undocumented) compilation.createChildCompiler() method, which is used to kick off the worker script compilations and appears to be automatically copying the main compilation plugin hook callbacks to each of the child compilations.

This was causing problems because one of the main compilation's plugins is the ContextReplacementPlugin, which bundles all the worker fallbacks into the dynamic require() in editorSimpleWorker.js. I didn't realise that this file is also referenced from within the worker scripts themselves (via the import * as worker from '...editor.worker.js' calls in the worker entry point files). This means that all the workers were also inheriting the same context factory, so all the worker fallbacks were being bundled into each compiled worker script even though that code path will never actually be hit from within the worker script.

I fixed this by applying an empty ContextReplacementPlugin to each of the worker script child compilations, which prevents the fallback implementations from being bundled into the compiled worker scripts. I also added a webpack.optimize.LimitChunkCountPlugin() to make sure that it doesn't generate an extra pointless chunk for the empty context factory that is used for the dynamic require() call.

Hopefully that should be enough to get it working without the babel transform – give me a shout if you hit any more problems!

@alexdima
Copy link
Member

@timkendrick

This works great now!

Thank you very much ❤️ !

@alexdima alexdima merged commit 53ad835 into microsoft:master Mar 20, 2018
@timkendrick
Copy link
Contributor Author

No problem, glad to help! Although I personally wouldn't ship this just yet…

The more I think about it the more I feel it would be safer to distribute this as a separate npm package that has a peerDependency on both monaco-editor and webpack, and it should grab the internal webpack plugins from its own webpack imports rather than having the user pass in their own webpack instance.

As it currently stands, there's an implicit dependency on webpack with no version checks to ensure that breaking webpack changes don't also break this plugin, which sounds dangerous to me.

Also, I feel that the current editor.main vs editor.api distinction could be improved a bit (although that's probably easier to change in a backwards-compatible way) – plus as stated above, I think the plugin options API could benefit from being a bit better specified.

What do you think?

@alexdima
Copy link
Member

@timkendrick

👍 I agree. Do you want to please set up a repository and a npm module? I believe you should own it, as I am a webpack noob. I can delete the code from this repository, and I can ship a new editor version with the change in self.require => require... and a couple other changes from regressions... I can also point to the webpack plugin you would create in documentation and readme.

Regarding editor.main vs editor.api, I think it is ok to have the most common use-case be the most simple one. I believe the most common use-case is to use everything we ship, aka editor.main (all bells & whistles included, no need to understand a lot of things). Advanced users can choose to have a smaller footprint by customising the way they package and they need to move from importing monaco-editor to importing monaco-editor/esm/vs/editor/editor.api.

@timkendrick
Copy link
Contributor Author

I think it might be better if someone over your side of the fence were to to own the plugin, for a couple of reasons:

  • The webpack plugin ties in closely with monaco-editor implementation details, so pushing an update to monaco-editor might require a simultaneous update to monaco-editor-webpack-plugin. It makes sense to have the same people publishing both, to allow a seamless upgrade path for users.
  • I won't personally have the time to maintain it (this is the main reason 😉)

Is there someone over at Microsoft who has more experience with webpack stuff maybe? I'm happy to help out with code comments/detailed explanations/general handover of how the plugin currently works if you guys want to understand how it works or maybe reimplement it, but I'm just doing this in my spare time so I sadly can't commit to the long-term maintenance burden of owning the repo/module.

As for editor.main vs editor.api – sure, that makes sense. It might be a good idea to look over some of the feature names in features.js and pick some better names, or at least standardise capitalization etc – the property keys of the main exported object are what is used for the features: ['feature1', 'feature2'] constructor option.

@mikegreiling
Copy link
Contributor

Is there someone over at Microsoft who has more experience with webpack stuff maybe?

paging @TheLarkInn 😁

@TheLarkInn
Copy link
Member

👋 @alexandrudima feel free to ping me on teams if needed also (alias: selarkin). I haven't had time since initial contact and working with @rebornix to dive back into this however I can help where needed.

@mikegreiling
Copy link
Contributor

@alexandrudima will the next published version include this webpack plugin?

@alexdima
Copy link
Member

Sorry I was offline for a bit. I am working on securing https://www.npmjs.com/package/monaco-editor-loader and following the MS procedures for setting up a new repo, etc...

@timkendrick
Copy link
Contributor Author

timkendrick commented Mar 26, 2018 via email

@moranje
Copy link

moranje commented Mar 31, 2018

Heads up. It looks like this plugin is webpack 3.6.0 incompatible. It's because of the following line of code in AddWorkerEntryPointPlugin.js:

compiler.hooks.make.tapAsync('AddWorkerEntryPointPlugin', (compilation, callback) => {}); // compiler.hooks.make is undefined

I'm unsure if there is an easy fix, my webpack skills are limited. Webpack 3.6.0 is still the default bundler for vue projects, I'm unsure about others.

@timkendrick
Copy link
Contributor Author

@moranje This is probably due to webpack's (largely undocumented) compiler hooks API changing between v3 and v4. The v3 style would be the following:

compiler.plugin('make', (compilation, callback) => { ... });

…as far as I recall, this will work in webpack v4 but emits cryptic deprecation warnings talking about Tapable. I guess a more thorough solution would be to feature-detect the existence of compiler.hooks.make and use the correct form.

Either way, this reinforces the fact that the plugin should probably have an explicit (peer) dependency on webpack that only permits usage with specific webpack versions.

@alexdima
Copy link
Member

https://www.npmjs.com/package/monaco-editor-webpack-plugin
https://github.com/Microsoft/monaco-editor-webpack-plugin

@kevinramharak
Copy link

@timkendrick

so trying what you suggested gets me the following error:

/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/monaco-editor-webpack-plugin/plugins/AddWorkerEntryPointPlugin.js:25
        new webpack.webworker.WebWorkerTemplatePlugin(),
                              ^

TypeError: Cannot read property 'WebWorkerTemplatePlugin' of undefined
    at Compiler.compiler.plugin (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/monaco-editor-webpack-plugin/plugins/AddWorkerEntryPointPlugin.js:25:31)
    at Compiler.applyPluginsParallel (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/tapable/lib/Tapable.js:293:14)
    at /home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/Compiler.js:488:8
    at Compiler.applyPluginsAsyncSeries (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/tapable/lib/Tapable.js:195:46)
    at Compiler.compile (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/Compiler.js:481:7)
    at /home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/Compiler.js:236:10
    at Compiler.readRecords (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/Compiler.js:388:10)
    at /home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/Compiler.js:233:9
    at next (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/tapable/lib/Tapable.js:202:11)
    at Compiler.compiler.plugin (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/CachePlugin.js:35:59)
    at Compiler.applyPluginsAsyncSeries (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/tapable/lib/Tapable.js:206:13)
    at /home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/Compiler.js:230:8
    at next (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/tapable/lib/Tapable.js:202:11)
    at Compiler.compiler.plugin (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/node/NodeEnvironmentPlugin.js:21:4)
    at Compiler.applyPluginsAsyncSeries (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/tapable/lib/Tapable.js:206:13)
    at Compiler.run (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/lib/Compiler.js:227:7)
    at processOptions (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/bin/webpack.js:359:12)
    at Object.<anonymous> (/home/kevin/Projects/GoogleAppsScript-IDE/node_modules/webpack/bin/webpack.js:363:1)
    at Module._compile (internal/modules/cjs/loader.js:654:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:665:10)
    at Module.load (internal/modules/cjs/loader.js:566:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:506:12)
    at Function.Module._load (internal/modules/cjs/loader.js:498:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:695:10)
    at startup (internal/bootstrap/node.js:201:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:516:3)

I was not expecting much but, do you have any suggestions where to start looking for a fix?

@kevinramharak
Copy link

ok so this comment: #18 (comment)
helped me out a lot. I have no idea what is happening exactly but the build is working. It does show a warning tough:

WARNING in ./~/monaco-editor/esm/vs/editor/common/services/editorSimpleWorker.js
378:12-387:17 Critical dependency: the request of a dependency is an expression

@mlajtos
Copy link

mlajtos commented Apr 16, 2018

Same here. Version 0.12

WARNING in ./node_modules/monaco-editor/esm/vs/editor/common/services/editorSimpleWorker.js
378:12-387:17 Critical dependency: the request of a dependency is an expression
 @ ./node_modules/monaco-editor/esm/vs/editor/common/services/editorSimpleWorker.js
 @ ./node_modules/monaco-editor/esm/vs/editor/editor.worker.js
 @ multi (webpack)-dev-server/client?http://localhost:8080 webpack/hot/dev-server monaco-editor/esm/vs/editor/editor.worker.js

@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants