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

iconv-loader webpack issue #18

Closed
iongion opened this issue Jun 21, 2017 · 19 comments
Closed

iconv-loader webpack issue #18

iongion opened this issue Jun 21, 2017 · 19 comments

Comments

@iongion
Copy link

iongion commented Jun 21, 2017

As you are aware, this works if library found is iconv, but what if the fallback is on iconv-lite ?

'use strict';

var iconv_package;
var Iconv;

try {
    // this is to fool browserify so it doesn't try (in vain) to install iconv.
    iconv_package = 'iconv';
    Iconv = require(iconv_package).Iconv;
} catch (E) {
    // node-iconv not present
}

module.exports = Iconv;

webpack output

WARNING in ./~/encoding/lib/iconv-loader.js
9:12-34 Critical dependency: the request of a dependency is an expression`

I am using webpack more like a bundler to ship single-file functions at AWS lambda for execution(there is no npm available there) and to also bundle dependencies(binary files are bundled too, by a manual script I maintain)

This change removes any issues:

'use strict';
module.exports = require('iconv-lite').Iconv;

But it will probably blow-apart when the dep is iconv and not iconv-lite, I don't know how to solve this only for my project.

What if the wrapper is more like this ?

'use strict';
var Iconv = null;
// this is to fool browserify so it doesn't try (in vain) to install iconv.
try {
  Iconv = require('iconv-lite').Iconv;
} catch (LE) {
  try {
    Iconv = require('iconv').Iconv;
  } catch (E) {
    // node-iconv not present
  }
}
module.exports = Iconv;
@JCharante
Copy link

JCharante commented Jul 25, 2017

I have the same issue

@ThomasVuillaume
Copy link

Same issue here

@pepihasenfuss
Copy link

Thank you, it worked for me!

@iongion
Copy link
Author

iongion commented Aug 2, 2017

@pepihasenfuss beware, it might still create issues if you use things such as node-fetch, I've got that and when I apply my replacement, this explodes:

TypeError: Iconv is not a constructor
    at convertIconv (W:\Workspace\swingman\node_modules\encoding\lib\encoding.js:76:13)
    at convert (W:\Workspace\swingman\node_modules\encoding\lib\encoding.js:39:22)
    at Body._convert (W:\Workspace\swingman\node_modules\node-fetch\lib\body.js:221:9)
    at Gunzip.<anonymous> (W:\Workspace\swingman\node_modules\node-fetch\lib\body.js:148:17)

Following up inside node_modules/encoding/lib/encoding.js I see this:

// Load Iconv from an external file to be able to disable Iconv for webpack
// Add /\/iconv-loader$/ to webpack.IgnorePlugin to ignore it
var Iconv = require('./iconv-loader');

I do not think this library is built to fallback gracefully from one to another, judging by the function argument useLite function convert(str, to, from, useLite) - ... but what If an external library such as node-fetch calls this with useLite = false ... and there is no iconv installed, and this is exactly the case I am in.

CORRECTION

The exception I get is ignorable, due to this code:

        try {
            result = convertIconv(str, to, from);
        } catch (E) {
            console.error(E);
            try {
                result = convertIconvLite(str, to, from);
            } catch (E) {
                console.error(E);
                result = str;
            }
        }

In the fallback(first try..catch), it should not be console.error but more of a notice
So scratch all, patch still works and I will need to change it so that it removes the console.error too!

Until the author has time for this, I have made a simple patch in vanilla JS, that you can call in the postinstall, save this in a postinstall.js

// IIFE
(function() {

'use strict';

// node
var fs = require('fs');
var path = require('path');

// Patch encoding module due to iconv issues -> make it use iconv-lite
(function() {
  var PATCH_VERSION = '0.1.12';
  var PATCH_MODULE = 'encoding';
  var PATCH_REASON = 'Use iconv-lite instead of iconv, helpful for webpack bundling';
  console.log('patching `%s`(%s) module', PATCH_MODULE, PATCH_VERSION);
  var pathToModule = path.join(path.dirname(__dirname), 'node_modules', PATCH_MODULE);
  var pathToModulePackage = path.join(pathToModule, 'package.json');
  var pathToModulePatchedFile1 = path.join(pathToModule, 'lib/iconv-loader.js');
  var pathToModulePatchedFile2 = path.join(pathToModule, 'lib/encoding.js');
  var moduleInfo = require(pathToModulePackage);
  if (moduleInfo.version !== PATCH_VERSION) {
    console.error(
      'patching `encoding` failed - expected `%s` but detected `%s`',
      PATCH_VERSION,
      moduleInfo.version
    );
    process.exit(1);
  }
  var contents;
  if (fs.existsSync(pathToModulePatchedFile1)) {
    contents = [
      '\'use strict\';',
      'module.exports = require(\'iconv-lite\');',
      '',
    ].join('\n');
    fs.writeFileSync(pathToModulePatchedFile1, contents);
  } else {
    console.error('patching `%s` failed because the file does not exist in', PATCH_MODULE, pathToModule);
    process.exit(1);
  }
  if (fs.existsSync(pathToModulePatchedFile2)) {
    contents = fs.readFileSync(pathToModulePatchedFile2).toString();
    contents = contents.replace('console.error(E);','');
    fs.writeFileSync(pathToModulePatchedFile2, contents);
  } else {
    console.error('patching `%s` failed because the file does not exist in', PATCH_MODULE, pathToModule);
    process.exit(1);
  }
  console.log('patching `%s`, reason: `%s` - completed', PATCH_MODULE, PATCH_REASON);
})();

})(this);

In your package.json:

...
  "scripts": {
    ...
    "postinstall": "node ./postinstall.js"
  }
...

@daliborgogic
Copy link

daliborgogic commented Aug 8, 2017

@iongion Error: Cannot find module '/home/dlbr/dev/node_modules/encoding/package.json'

Correct path is:

...
var pathToModule = path.join(__dirname, 'node_modules', PATCH_MODULE);
...

package.json:

...
  "scripts": {
   ...
    "postinstall": "node postinstall.js"
  }
...

@MehdiITService
Copy link

it worked for me changing the content of the file 'iconv-loader.js' to :
'use strict';
module.exports = require('iconv-lite').Iconv;

thank u very much

@aleixsuau
Copy link

aleixsuau commented Feb 1, 2018

Thanks @iongion , your solution (...require('iconv-lite').Iconv) stops the "Critical dependency: the request..." alert, but there is another effect that remains: webpack keeps adding the entire iconv-lite to the final bundle, which is 493kb. (angular cli project) :(

Any idea on this?

@iongion
Copy link
Author

iongion commented Feb 1, 2018

@aleixsuau It depends on your use case, you might not be able to avoid it because it is really needed by your project requirements, you could give it a try as it is described here https://remarkablemark.org/blog/2017/02/25/webpack-ignore-module

@andriy-f
Copy link

Why not just add new webpack.IgnorePlugin(/\/iconv-loader$/), to webpack? It removes warnign

@LukasBombach
Copy link

@andriy-f this basically mean not using this pliugin which cannot be the solution for using this plugin

@ofhouse
Copy link

ofhouse commented Mar 18, 2019

@iongion Thank you for the patch 👍

I am using it in a yarn workspace environment, so I've updated the resolution of the package in the script with require.resolve():

var pathToModule = path.join(path.dirname(__dirname), 'node_modules', PATCH_MODULE);
to
var pathToModule = path.dirname(require.resolve(PATCH_MODULE + '/package.json'));

Here is the full updated script:
// IIFE
(function() {

'use strict';

// node
var fs = require('fs');
var path = require('path');

// Patch encoding module due to iconv issues -> make it use iconv-lite
(function() {
  var PATCH_VERSION = '0.1.12';
  var PATCH_MODULE = 'encoding';
  var PATCH_REASON = 'Use iconv-lite instead of iconv, helpful for webpack bundling';
  console.log('patching `%s`(%s) module', PATCH_MODULE, PATCH_VERSION);
  var pathToModule = path.dirname(require.resolve(PATCH_MODULE + '/package.json'));
  var pathToModulePackage = path.join(pathToModule, 'package.json');
  var pathToModulePatchedFile1 = path.join(pathToModule, 'lib/iconv-loader.js');
  var pathToModulePatchedFile2 = path.join(pathToModule, 'lib/encoding.js');
  var moduleInfo = require(pathToModulePackage);
  if (moduleInfo.version !== PATCH_VERSION) {
    console.error(
      'patching `encoding` failed - expected `%s` but detected `%s`',
      PATCH_VERSION,
      moduleInfo.version
    );
    process.exit(1);
  }
  var contents;
  if (fs.existsSync(pathToModulePatchedFile1)) {
    contents = [
      '\'use strict\';',
      'module.exports = require(\'iconv-lite\');',
      '',
    ].join('\n');
    fs.writeFileSync(pathToModulePatchedFile1, contents);
  } else {
    console.error('patching `%s` failed because the file does not exist in', PATCH_MODULE, pathToModule);
    process.exit(1);
  }
  if (fs.existsSync(pathToModulePatchedFile2)) {
    contents = fs.readFileSync(pathToModulePatchedFile2).toString();
    contents = contents.replace('console.error(E);','');
    fs.writeFileSync(pathToModulePatchedFile2, contents);
  } else {
    console.error('patching `%s` failed because the file does not exist in', PATCH_MODULE, pathToModule);
    process.exit(1);
  }
  console.log('patching `%s`, reason: `%s` - completed', PATCH_MODULE, PATCH_REASON);
})();

})(this);

dajohi added a commit to dajohi/decrediton that referenced this issue May 1, 2019
dajohi added a commit to dajohi/decrediton that referenced this issue May 1, 2019
dajohi added a commit to dajohi/decrediton that referenced this issue May 1, 2019
@jailsonpaca
Copy link

it worked for me changing the content of the file 'iconv-loader.js' to :
'use strict';
module.exports = require('iconv-lite').Iconv;

thank u very much

me too!

@simonmaass
Copy link

any update on this? will this be fixed soon?

@agustiza
Copy link

Second on this petition

@ton77v
Copy link

ton77v commented Jul 10, 2020

In your package.json:

...
  "scripts": {
    ...
    "postinstall": "node ./postinstall.js"
  }
...

Thanks a lot! This worked for me when I had the same issue with Sentry for Electron

I'll add a note for others who will find this =)

  1. should write "node postinstall.js" without './'
  2. I also had to change from
    var pathToModule = path.join(path.dirname(__dirname), 'node_modules', PATCH_MODULE)
    to
    var pathToModule = path.join(__dirname, 'node_modules', PATCH_MODULE)

@andris9
Copy link
Owner

andris9 commented Jul 10, 2020

Just wondering, why is this module even used by anyone? The goal for this module was to load compiled node-iconv and if not available, fallback to iconv-lite. If you never need node-iconv, then you could go straight to iconv-lite and skip this wrapper module entirely.

@ton77v
Copy link

ton77v commented Jul 10, 2020

Just wondering, why is this module even used by anyone? The goal for this module was to load compiled node-iconv and if not available, fallback to iconv-lite. If you never need node-iconv, then you could go straight to iconv-lite and skip this wrapper module entirely.

Absolutely no idea =) My case the warning appeared when I started to use deep import in https://github.com/getsentry/sentry-electron

Seems that the problem exists for a while there

@andris9
Copy link
Owner

andris9 commented Jul 10, 2020

It appears that this module is not used for its original purpose anymore anywhere so I guess I could just remove the iconv thing and keep iconv-lite like a normal dependency, so there would be no loader shenanigans needed.

@andris9
Copy link
Owner

andris9 commented Jul 10, 2020

fixed in v0.1.13

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 a pull request may close this issue.