Fix: Absolute paths being used when expose-loader exposes more than one global for a library

[e-pavlica]

When using expose launcher to expose more than one global for a library, e.g.

loaders: [{
  test: require.resolve('jquery'),
  use: [
    {
      loader: 'expose-loader',
      options: '$'
    },
    {
      loader: 'expose-loader',
      options: 'jQuery'
    }
  ]
}]

the remainingRequest for one of the sources is a reference to the already-loaded libaray:
/Users/foo/projects/bar/node_modules/expose-loader/index.js?jQuery!/Users/foo/projects/bar/node_modules/jquery/dist/jquery.js

Since the existing logic doesn't change the path of the expose-loader/index.js file from absolute to relative, and this value is used in the webpack chunkhash value, we were experiencing miss-matched hashes for the same file if built from a differently-named directory.

The result of this PR would change the require in the return from this:

require("/Users/foo/projects/bar/node_modules/expose-loader/index.js?jQuery!./jquery.js")

to this:

require("./../../expose-loader/index.js?jQuery!./jquery.js")

tl;dr

• adds an additional check for the expose-loader path
• if present, replaces the absolute expose-loader path with a relative path

[jsf-clabot]

All committers have signed the CLA.

[michael-ciniawsky]

cc @timse Sry for pinging :), but I'm not too familiar with the internals of this loader and don't use it in an active project atm to test and it is related to cache invalidation aswell 😛 would you mind reviewing this PR ? 🙏

[timse]

Sure can do, I am not sure I fully understand the problem.

The resolution of non-absolute paths should not be handled by the loader - and I can't reproduce your problem locally @xicreative
Can you give a reproducible example?
Until then the PR seams unnecessary to me.

[e-pavlica]

Yeah, here's the situation that we ran into that inspired this PR:
Our site is served by two load-balanced servers, which both run webpack separately and host their own assets. You'd expect the chuckhash value for assets to be the same on both servers since the code is identical, but because the absolute path to these files wasn't identical, neither was the hash. If our load balancer pointed traffic to the other server, we'd get a 404 because of the miss-matched hash in the filename.

Here's a repo to demo the issue: https://github.com/xicreative/demo-webpack-expose-loader

Steps:

• yarn install && yarn run build
• Note the hashes on manifest and vendor files in dist
• Change the repo directory name
• rm -r dist && yarn run build
• Compare the new hashes on manifest and vendor files in dist

This behaviour does NOT happen if one of the objects in the use array is commented out, e.g.

loaders: [{
  test: require.resolve('jquery'),
  use: [
    {
      loader: 'expose-loader',
      options: '$'
    },
    // {
    //   loader: 'expose-loader',
    //   options: 'jQuery'
    // }
  ]
}]

This MAY be caused in part by the combination of expose-loader and CommonsChunkPlugin, but removing the absolute path generated by expose-loader solved this issue for us.

[timse]

hmm fair enough - i guess there is some weird stuff happening when the hash gets calculated.

However for me this seems to be a flaw in webpack itself and should not be handled by every loader.

Any opinions @d3viant0ne @michael-ciniawsky ?

[timse]

If it is not webpacks responsibility then I would rather argue to allow for multiple globals to be exposing the same module.

E.g. by allowing

options: "jQuery,$"

or

options: {
   expose: ['jQuery', '$']
}

[ghost]

Since 'needs repro' is still flagged:

My webpack4 setup has suddenly decided to be 'unhappy' with me, choking on use of 'expose-loader' for jqeury.

With this in config -- which had, until yesterday, been working great --

exports.provideJquery = () => ({
	plugins: [
		new webpack.ProvidePlugin({
			'window.jQuery': 'jquery',
			'window.$':      'jquery',
			'jQuery':        'jquery',
			'$':             'jquery',
		})
	],
});
exports.exposeJquery = () => ({
	module: {
		rules: [
			{
				test: require.resolve('jquery'),
				use: [
					{
						loader: 'expose-loader',
						options: '!expose-loader?jQuery'
					},
					{
						loader: 'expose-loader',
						options: '!expose-loader?$'
					}
				]
			}
		]
	}
});

I'm now seeing

ERROR in ./node_modules/jquery/dist/jquery.js (./node_modules/expose-loader!./node_modules/expose-loader?$!./node_modules/jquery/dist/jquery.js)
Module build failed: Error: query parameter is missing
    at Object.module.exports.pitch (/opt/test/node_modules/expose-loader/index.js:34:24)
 @ ./node_modules/jquery/dist/jquery.js-exposed 1:51-209
 @ ./assets/js/login.js
 @ multi ./assets/js/login.js ./assets/scss/login.scss
error An unexpected error occurred: "Command failed.
Exit code: 2
Command: sh
Arguments: -c webpack --env development --progress

if I comment out

//					{
//						loader: 'expose-loader',
//						options: '!expose-loader?jQuery'
//					},

no more issues.

why it 'suddenly' started being an issue, dunno

[the-teacher]

@xicreative Thanks! Your solution works! 👍

@evilebottnawi @michael-ciniawsky @d3viant0ne Hey guys when will it be merged? Could you please take care about it. This issue is very important for users who use expose-loader and want to provide Long Term Caching for their projects. It's just unacceptable to have different chunk hashes due differences in paths.

If you need any help with testing or reproducing, please, just let us know.
Thanks!

[alexander-akait]

@xicreative can you rebase you PR?

[e-pavlica]

@evilebottnawi Rebase in progress. Also looking at ways to provide options as @timse suggested, as that seems more efficient.

[e-pavlica]

@evilebottnawi This is ready to be re-reviewed. I added some tests that may be a little heavy-handed but I wanted to make the sure the desired globals were actually being exposed on the window object. 🤷‍♂️

[NathanKleekamp]

Can we get an update on any progress getting this merged?

[NathanKleekamp]

Hey @xicreative have you heard back on this PR? We've been using this fork in production for some time, and it's been working great for us.

[e-pavlica]

@NathanKleekamp I haven't... it's in the hands of @evilebottnawi & @ooflorent at the moment.

[NathanKleekamp]

Hi @xicreative, @evilebottnawi, and @ooflorent. I just wanted to ping the PR again after the New Year in the hopes we can get the changes approved and merged. Thanks!

[NathanKleekamp]

Just thought I'd ping @xicreative, @evilebottnawi, and @ooflorent again to see if any progress has been made getting this PR merged or what, if anything, I can do to help get it merged. Thanks!

[e-pavlica]

Sorry @NathanKleekamp this one's out of my control.
@michael-ciniawsky @d3viant0ne are either of you able to review/merge?

[garygreen]

Would it make more sense to use loaderUtils.stringifyRequest ? This seems to be the recommended way to resolve to relative paths in Webpack. Maybe it's not applicable here, but thought I'd throw suggestion out there.

Would be great to see a bit more movement on this PR as it does break consistent hashing across different machines. See this article: https://medium.com/@the_teacher/webpack-different-assets-hashes-on-different-machines-the-problem-the-solution-ec6383983b99