Import loader create invalid variable name based off package name

[chrisrink]

Try importing jquery-migrate into a legacy jquery plugin, and you will get a compile error:

`/node_modules/imports-loader/index.js?jQuery=jquery,this=>window,"jquery-igrate"!...node_modules/vh.jquery.ba-bbq/jquery.ba-bbq.js Unexpected token (4:4) You may need an appropriate loader to handle this file type.	var jQuery = require("jquery");	(function() {	var "jquery-migrate" = require("\"jquery-migrate\"");
/*!`

My workaround is to create an alias but it would be nice if the import loader replaced invalid variable name characters with underscores or something.

[michael-ciniawsky]

/node_modules/imports-loader/index.js?jQuery=jquery,this=>window,"jquery-igrate"!

"jquery-igrate" => "jquery-migrate" ?

Please provide more info about your current setup (webpack.confg.js etc)

[bebraw]

@chrisrink Can you set up a small repository showcasing the issue?

[chrisrink]

Here is a repo you can reproduce with:

https://github.com/chrisrink/webpack-import-issue

I added a few comments in the webpack.config to highlight the issue.

[alexander-akait]

@bebraw seems this error also present in webpack, using

new webpack.ProvidePlugin({
    'window.jQuery-test': 'jquery',
})

Output:

...
/* WEBPACK VAR INJECTION */(function(__webpack_provided_window_dot_jQuery-test) {/* harmony import */
...

and error Uncaught SyntaxError: Unexpected token -

[alexander-akait]

Solution:

1. Won't fix - just add caveat about this in docs.
2. Do it as in webpack https://github.com/webpack/webpack/blob/f66439e2b736db381481d42988076f39d30d77a5/lib/dependencies/HarmonyModulesHelpers.js#L16 (but it is can't working with some scripts try to access using globals['jquery-migrate'], this['jquery-migrate'], windows['jquery-migrate'])
3. Add new operator (example jquery-migrate>jquerymigrate) to allow using own names.

jquery-migrate>jquerymigrate -> var jquerymigrate = require('jquery-migrate')

jquery-migrate>variable.jquerymigrate -> var variable = (variable || {});variable.jquerymigrate = require('jquery-migrate')

[bebraw]

Yeah, I can see why this fails. The problem is that if you rewrite the name, you could theoretically end up with a name clash (it maps to an existing name). I think going through a string based lookup is a good way to solve this as it avoids the problem. Having a new operator for this and pushing the problem to the user doesn't feel right (could be done regardless, though).

It would be good to fix ProvidePlugin while at it. Maybe the solution could be shared.

[alexander-akait]

@bebraw

I think going through a string based lookup is a good way to solve this as it avoids the problem.

Little did not understand, can you give more details?

[bebraw]

@evilebottnawi (this['jquery-migrate']) or some other variant over (jquery-migrate) (invalid syntax).

[alexander-akait]

@bebraw example. Now:

// Improted
var library = require('library');

// Code in library
function Test() {
   library.on('', function() {});
}

var test = new Test();

After your suggestions

// Improted
(this['library'] = require('library'));

// Code in library
function Test() {
   library.on('', function() {});
}

Test(); // Don't work in `strict` mode and many modern browsers without `strict` mode
var test = new Test(); //  Don't work, but work in chrome console :smile:

[alexander-akait]

For me best solution add new operator, it is allow to see how library using other library (jqueryMigrate, jquerymigrate, this.jquerymigrate, window.jquerymigrate) and setup this right.

[bebraw]

@evilebottnawi I did not mean it literally. Most likely you need something else than this depending on the surrounding code.

[alexander-akait]

@bebraw i tried many solution, no found universal :disappointed:

[bebraw]

@evilebottnawi I guess the way that remains is to find an acceptable replace for -. This feels like a common enough problem so there's likely some convention somewhere. Maybe _ and then detect clashes (give warning) to play it safe?

[alexander-akait]

Solution (work in ES3, ES5 and ES5-strict, based on http://es5.github.io/#x10.4.2):

var globalContext = (function(){ return this || (1,eval)('this') })();
globalContext['my-variable'] = require('my-varialbe');

[bebraw]

@evilebottnawi Ok, that eval is fairly tricky. Maybe better wait for more feedback on this to see if we can eliminate that.

[alexander-akait]

@bebraw I think we are unlikely to get many answers here, a rare use case, in fact, i don't see in my decision what is really bad, yep eval is bad, but it is allow get global context in whatever environments.

[bebraw]

@evilebottnawi Ok, basically it's fine by me as long as the reason is documented well like that.

[alexander-akait]

@d3viant0ne what do your this about this?

[alexander-akait]

Other solution https://github.com/zloirock/core-js/blob/master/modules/_global.js

[Jessidhia]

This discussion is going in the wrong direction. imports-loader is for injecting values in this module's scope, not put things in global just because you can't find a local name.

[alexander-akait]

@Kovensky your have another solution for this?

[alexander-akait]

@Kovensky many people use imports-loader for injecting values :smile: If your know best solution for injecting values in scripts, provide this here and let's add this in docs.

[Jessidhia]

If you want to put it on a global, use expose-loader. imports-loader is for scoped values.

This is especially relevant for jquery-migrate as it is a side-effect-only script (modifies the global (or imports-loader-provided) jQuery) -- there's not even anything to put in global, just make sure it runs beforehand.

[alexander-akait]

@Kovensky hm, you're right, i'm ashamed, not familiar with expose-loader, because don't use libraries using the global object :smile: Seems - to _ is good solution (also add docs about this).

[alexander-akait]

@bebraw friendly ping

[bebraw]

Seems - to _ is good solution (also add docs about this).

That's fine. 👍