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

Update VirtualModulesPlugin from upstream #136

Closed
wants to merge 11 commits into from

Conversation

syvb
Copy link
Contributor

@syvb syvb commented Sep 19, 2020

tl;dr: Webpack 5 support, fixes TypeError: Cannot read property 'data' of undefined

This updates the internal VirtualModulesPlugin, which is copied from webpack-virtual-modules, which was originally added by @kaisermann. These changes allow for using Webpack 5 with svelte-loader. The changes of #125, which have also been fixed upstream, have been preserved. See #59 (comment) for the background on why the source code is just copied over instead of added as a dependency. Due to the svelte-loader specific changes already in virtual.js, I have manually gone through the commit history of webpack-virtual-modules and applied relevant changes instead of just copy-pasting the new source code. There is also a small change to index.js to prevent the error handling code from itself throwing errors, since that was causing some (now fixed) troubles when debugging my changes.

This also adds in the literal text of the license of webpack-virtual-modules, to a newly created LICENSE file, since "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software".

Tests and lints pass, and I have verified this works with both Webpack 4 and Webpack 5

[continuation from #132, since GH won't let me re-open it]

Closes #133

This updates the internal VirtualModulesPlugin, which is copied from
https://github.com/sysgears/webpack-virtual-modules. I went through all
commits since this virtual.js was added, and applied them manually to
the svelte-loader version where applicable. These upstream changes
allow for using Webpack 5 with svelte-loader.

This also adds a LICENSE file to comply with the requirements of the MIT
license for the plugin files. The LICENSE file also adds in the text of
MIT license that svelte-loader is licensed under (with a different
copyright line, since svelte-loader wasn't written by SysGears)
lib/virtual.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
lib/virtual.js Show resolved Hide resolved
@syvb
Copy link
Contributor Author

syvb commented Oct 12, 2020

@benmccann I've changed all uses of double quotes to single quotes.

lib/virtual.js Outdated Show resolved Hide resolved
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@larixer
Copy link

larixer commented Oct 15, 2020

@Smittyvb You might probably need to check whether you affected by this Issue:
sysgears/webpack-virtual-modules#68
and related PR:
sysgears/webpack-virtual-modules#69

@syvb
Copy link
Contributor Author

syvb commented Oct 21, 2020

I've implemented sysgears/webpack-virtual-modules@4894019 from upstream, which implements watch mode support in a way that is compatible with Webpack 5. So to recap:

lib/virtual.js Outdated Show resolved Hide resolved
Replaces all usage of var with let/const
lib/virtual.js Outdated Show resolved Hide resolved
lib/virtual.js Outdated Show resolved Hide resolved
@larixer
Copy link

larixer commented Oct 27, 2020

@Smittyvb Guys, if you are up to it, I would be open to PR to expose some interface inside webpack-virtual-modules to make it easier for you to consume it directly inside svelte-loader, it might make future upgrades easier. Please consider this as an option now or in the future.

@syvb
Copy link
Contributor Author

syvb commented Oct 27, 2020

@larixer I'm working on that: https://github.com/Smittyvb/svelte-loader/tree/wvm-as-dep

@benmccann
Copy link
Member

That would be great and I think would help get this PR checked in. I talked to some of the other maintainers about getting this PR checked in since I don't have much webpack background and haven't worked on this repo much, and I think there was some feeling that they'd like to see webpack 5 support be a bit cleaner/simpler. I'm not sure exactly how you would do that, but if there's a nice interface for webpack-virtual-modules to support this that sounds like it could help

@jquense
Copy link

jquense commented Oct 30, 2020

@larixer would also love to see something like that. I've forked the plugin a few times in order to allow bootstrapping in a loader.

if you are looking for ideas on an interface i've found this effective: https://github.com/4Catalyzer/astroturf/blob/v1/src/VirtualModulePlugin.ts#L99-L108

lib/virtual.js Outdated
function getData(storage, key) {
const data = storage._data /* webpack 5 */ || storage.data /* webpack 4 */;
if (data instanceof Map) {
return storage.data.get(key);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be data.get(key)

Copy link

@deleonio deleonio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code advices

lib/virtual.js Outdated
return data.data[key];
}

function setData(backendOrStorage, key, valueFactory) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is not nice. Better looks like getData, a webpack switch and a generic code

@deleonio
Copy link

Oh sorry - I will fix the bug - if I can,

@deleonio
Copy link

Hello @Smittyvb, I fixed it. Please merge it again syvb#3

fix: 'backendOrStorage' is not defined  no-undef
@antonlyap
Copy link

Hello everyone, it looks like there is something wrong with style handling on this branch when svelte-preprocess is enabled. I get errors like this for all of my components which have styles:

Module not found: Error: Can't resolve '/path-to-my-app/src/components/Header/index.svelte.css' in '/path-to-my-app/src/components/Header'

The directory structure for the Header component (and all others) is as follows:

Header
  - index.scss
  - index.svelte

And the style is connected to the component like this: <style type="text/scss" src="index.scss"></style>.

Here is my Webpack configuration:

{
  entry, // custom generated list of entry points
  mode: 'production',
  output: {
    path: path.join(projectRoot, 'dist'),
    filename: '[name].js',
  },
  resolve: {
    alias: {
      svelte: path.resolve(projectRoot, 'node_modules', 'svelte'),
    },
    extensions: ['.mjs', '.js', '.svelte', '.css'],
    mainFields: ['svelte', 'browser', 'module', 'main'],
  },
  module: {
    rules: [
      { test: /.m?js$/, type: 'javascript/auto' },
      {
        test: /\.svelte$/,
        use: {
          loader: 'svelte-loader',
          options: {
            hotReload: false,
            hydratable: true,
            preprocess: sveltePreprocess(), // imported from 'svelte-preprocess'
            dev: false,
            emitCss: true,
          },
        },
      },
      {
        test: /\.css$/,
        use: [MiniCssExtractPlugin.loader, 'css-loader'],
      },
    ],
  },
  plugins: [
    new MiniCssExtractPlugin({
      filename: '[name].css',
    }),
  ],
  optimization: {
    splitChunks: {
      chunks: 'all',
    },
  },
}

What fixes the error:

  • using the latest version from npm or the master branch from this repo and downgrading to Webpack 4
  • removing the preprocess option and converting the styles into CSS

What doesn't fix the error:

  • converting the styles into CSS
  • switching between Webpack v4 and v5

@deleonio
Copy link

I think the changed files have nothing to do with CSS. The misconduct with regard to webpack 5 and preprocess is a separate topic.

It would be cool if the PR comes so that Webpack5 can work in less complex situations.

@deleonio
Copy link

That is solved for us leanupjs/leanup#37.

@syvb
Copy link
Contributor Author

syvb commented Jan 11, 2021

Superseded by #146, which fixes the problem in a much better and more maintainable manner.

@syvb syvb closed this Jan 11, 2021
syvb added a commit to syvb/svelte-loader that referenced this pull request Jan 11, 2021
Before this licence was in the package.json but it wasn't actually in the repo. sveltejs#136 would have added a license but now sveltejs#146 is the future, and that PR clears up the license issue by not including webpack-virtual-modules in-repo, so it doesn't have to affect the LICENSE file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webpack 5. Error: TypeError: TypeError: Cannot read property 'data' of undefined
7 participants