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

fix(loader): pass emitFile to the child compilation (loaderContext.emitFile) #177

Merged

Conversation

piecyk
Copy link
Contributor

@piecyk piecyk commented Jun 12, 2018

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Looks like mini-css-extract-plugin when collecting sources it's replacing module dependencies
with new CssDependency as assets are loaded before. IMHO we can collect and add it back to updated module.

Use-case for this is when using hard-source-plugin and reading from cache can't find asset
mzgoddard/hard-source-webpack-plugin#367

Breaking Changes

No breaking changes, all current test's are passing

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Jun 12, 2018

CLA assistant check
All committers have signed the CLA.

src/loader.js Outdated
// Collect assets from modules
compilation.modules.forEach((module) => {
if (module.buildInfo && module.buildInfo.assets) {
assets = { ...assets, ...module.buildInfo.assets };
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use Object.assign(assets, module.buildInfo.assets) to reduce the GC stress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

src/index.js Outdated
if (!Array.isArray(content) && content != null) {
throw new Error(
`Exported value was not extracted as an array: ${JSON.stringify(
content
)}`
);
}

module.buildInfo = module.buildInfo || { assets: {} };
Copy link
Contributor

Choose a reason for hiding this comment

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

|| {} is enough since you add assets just right after this statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 👍

@alexander-akait
Copy link
Member

@piecyk Also please accept CLA, thanks!

@piecyk
Copy link
Contributor Author

piecyk commented Jun 18, 2018

Thanks @ooflorent for review 👏squash and merge!

@piecyk
Copy link
Contributor Author

piecyk commented Jun 18, 2018

Strange licence/cla Pending — Contributor License Agreement is not signed yet.
opening the link You have signed the CLA for webpack-contrib/mini-css-extract-plugin wat ?

@sokra
Copy link
Member

sokra commented Jun 20, 2018

Please add tests

@sokra
Copy link
Member

sokra commented Jun 20, 2018

Instead of assigning to module.assets use the emitFile loader api

@sokra
Copy link
Member

sokra commented Jun 20, 2018

It's also possible to hook into normalModuleLoader to change the emitFile method in the child compilation and pass through the call directly.

@michael-ciniawsky michael-ciniawsky added this to the 0.5.0 milestone Aug 6, 2018
@michael-ciniawsky michael-ciniawsky changed the title feat: collect assets from modules feat: collect module assets Aug 21, 2018
@michael-ciniawsky michael-ciniawsky removed the request for review from shellscape August 21, 2018 20:33
@michael-ciniawsky
Copy link
Member

Blunt, but whtat's the current status of this PR ? :)

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Need tests

@piecyk
Copy link
Contributor Author

piecyk commented Aug 23, 2018

Sorry for delay... thanks @sokra for tips, will try to finish it over the weekend or close it!

@surfaceowl
Copy link

@piecyk -- any update or timeline on this PR? Also there are some merge conflicts.

@alexander-akait
Copy link
Member

@piecyk can you rebase PR?

@michael-ciniawsky michael-ciniawsky modified the milestones: 0.5.0, 0.4.3 Sep 18, 2018
@renchap
Copy link

renchap commented Sep 18, 2018

It looks like this is creating a regression when using a manifest plugin (https://github.com/webdeveric/webpack-assets-manifest in my case).
Images, fonts and other assets referenced from CSS files are no longer present in manifest.json after upgrading from 0.4.2 to 0.4.3.
Is this intended and should there be a change in webpack-assets-manifest?

@Baune8D
Copy link

Baune8D commented Sep 20, 2018

It looks like this is creating a regression when using a manifest plugin (https://github.com/webdeveric/webpack-assets-manifest in my case).
Images, fonts and other assets referenced from CSS files are no longer present in manifest.json after upgrading from 0.4.2 to 0.4.3.
Is this intended and should there be a change in webpack-assets-manifest?

Also seeing this change.

Some assets are completely missing from the manifest, others are still in there but key values have changed.

Example manifest entry:

"images/create/wallet.png": "images/create/wallet.png"

to

"images/create/Create.scss": "images/create/wallet.png"

@alexander-akait
Copy link
Member

@Baune8D very strange, looks something wrong in plugin, we use standard webpack api and just allow loaders to emit

@renchap
Copy link

renchap commented Sep 20, 2018

I created webdeveric/webpack-assets-manifest#39 about this, but I am really unsure this is a manifest plugin bug. This change must have done something that changes what is outputed, and it is not documented.

@piecyk
Copy link
Contributor Author

piecyk commented Sep 21, 2018

Yeah, it looks like something wrong in webpack-assets-manifest Basic is trying to read the module.userRequest to get the original file name before hash ( or at least that how i understand it )

https://github.com/webdeveric/webpack-assets-manifest/blob/master/src/WebpackAssetsManifest.js#L548

@webdeveric
Copy link

I'm looking at this now to see if its something I can update on my end.

I am reading module.userRequest to get the original name and it was working fine in mini-css-extract-plugin 0.4.2 and below. Other loaders, like file-loader, currently work as expected.

When using 0.4.3, module.userRequest is set to the css file and not the asset (bg image, font, etc.).

@piecyk
Copy link
Contributor Author

piecyk commented Sep 22, 2018

Maybe getting emitted files bit differently will solve the issue webdeveric/webpack-assets-manifest#39 (comment)

@michael-ciniawsky michael-ciniawsky removed this from the 0.4.4 milestone Oct 2, 2018
weaverryan added a commit to symfony/webpack-encore that referenced this pull request Oct 18, 2018
… the dependencies (Lyrkan)

This PR was squashed before being merged into the master branch (closes #403).

Discussion
----------

Add new Travis jobs to test lowest/highest versions of all the dependencies

This PR adds two Travis jobs that allow to test the lowest and highest versions of all the dependencies (including the dev ones since the version ranges are used by the package helper).

I wasn't sure about the method to use for the lowest versions since we can't really guess them without calling the npm's registry API. So, if someone has a better idea... :)

Note that both jobs currently fail for the following reasons:

- Lowest versions: This isn't really an issue since it seems related to Webpack 4 for which we haven't released a version yet. We could increase the minimum version to match the one from the `yarn.lock` file before releasing.
- Highest versions: This is caused by the last version of the `mini-css-extract-plugin` (0.4.3) which doesn't seem to work well with the `webpack-manifest-plugin` anymore (see webpack-contrib/mini-css-extract-plugin#177 (comment)).

Closes #39.

Commits
-------

bcabda6 Add some missing 'return' statements
0161000 Remove hardcoded sass-loader version in addPackagesVersionConstraint test
7abea75 Add new Travis jobs to test lowest/highest versions of all the dependencies
@chrisdarroch
Copy link

chrisdarroch commented Nov 8, 2018

This change appears to have broken some assumptions in our webpack plugin, too. Our issue and exploration of the problem is written up here: https://bitbucket.org/atlassianlabs/atlassian-webresource-webpack-plugin/issues/52

After this change, a Module representing the CSS file will have a module.buildInfo.assets map, where the keys are the new filenames of the CSS's external file dependencies (images etc) and the values their Source.

Module {
  resource: 'style.css',
  buildInfo: {
    assets: {
      '12345.png': RawSource{...},
      '67890.svg': RawSource{...}
    }
  }
},
Module {
  resource: 'one.png',
  buildInfo: {}
},
Module {
  resource: 'two.svg',
  buildInfo: {}
}

Prior to this change, the asset metadata was stored directly on the Module for the external file dependency.

Module {
  resource: 'style.css',
  buildInfo: {}
},
Module {
  resource: 'one.png',
  buildInfo: {
    assets: {
      '12345.png': RawSource{...}
    }
  }
},
Module {
  resource: 'two.svg',
  buildInfo: {
    assets: {
      '67890.svg': RawSource{...}
    }
  }
}

The reason for this behaviour change is precisely because the emitFile method is now shared. The emitFile method is responsible for populating the buildInfo.assets map.

As a consequence, it's not easy to build a map of original filenames -> compiled filenames by iterating through all Modules involved in the compilation, since there is no obvious 1:1 mapping relationship between them.

If anybody has alternative suggestions for building the map of original filenames -> compiled filenames, I'm keen to hear them.

@chrisdarroch
Copy link

After reading through @renchap's @piecyk's and @webdeveric's exploration and fix in the webpack-assets-manifest plugin, I did something similar, with the same effect.

Instead of iterating through all Modules in the compilation, I instead hooked in to the normalModuleLoader and substituted the emitFile method with the following:

const assetNames = new Map();

compiler.hooks.compilation.tap('maybe a hack', compilation => {
    compilation.hooks.normalModuleLoader.tap('maybe a hack', (loaderContext, module) => {
        const { emitFile } = loaderContext;
        loaderContext.emitFile = (name, content, sourceMap) => {
            const originalName = module.userRequest;
            assetNames.set(originalName, name);

            return emitFile.call(module, name, content, sourceMap);
        };
    });
});

compiler.hooks.emit.tap('after emit', () => {
    console.log(assetNames); // logs originalFilepath -> compiledFilename
});

This does what I need it to, but as noted, it may be a hack. If there are better thoughts on how to do this better, let me know :)

karlvr added a commit to karlvr/webpack-manifest-plugin that referenced this pull request Nov 13, 2018
Resolves shellscape#167

mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
karlvr added a commit to karlvr/webpack-manifest-plugin that referenced this pull request Nov 13, 2018
Second attempt. Based on webdeveric/webpack-assets-manifest#40

Resolves shellscape#167

mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
mastilver pushed a commit to shellscape/webpack-manifest-plugin that referenced this pull request Sep 25, 2019
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177

fixes #167
mastilver pushed a commit to shellscape/webpack-manifest-plugin that referenced this pull request Sep 25, 2019
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177

fixes #167
mastilver pushed a commit to shellscape/webpack-manifest-plugin that referenced this pull request Sep 25, 2019
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177

fixes #167
mastilver pushed a commit to shellscape/webpack-manifest-plugin that referenced this pull request Sep 25, 2019
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177

fixes #167
mastilver pushed a commit to shellscape/webpack-manifest-plugin that referenced this pull request Sep 26, 2019
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177

fixes #167
karlvr added a commit to karlvr/webpack-manifest-plugin that referenced this pull request Jan 29, 2020
Resolves shellscape#167

mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
karlvr added a commit to karlvr/webpack-manifest-plugin that referenced this pull request Jan 29, 2020
Second attempt. Based on webdeveric/webpack-assets-manifest#40

Resolves shellscape#167

mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet