farfromrefug / react-xterm

52 stars 27 forks source link

Dynamic requires causes webpack to include test files in bundle #7

Open davej opened 6 years ago

davej commented 6 years ago

react-xterm contains dynamic requires which causes Webpack to be overly aggressive when bundling and include all the addons and corresponding tests:

this.props.addons.forEach(s => {
  const addon = require(`xterm/dist/addons/${s}/${s}`);
  Terminal.applyAddon(addon);
});

Causing the following error:

* chai in ./node_modules/xterm/dist/addons/attach/attach.test.js, ./node_modules/xterm/dist/addons/fit/fit.test.js and 4 others
* express in ./node_modules/xterm/dist/addons/zmodem/demo/app.js
* express-ws in ./node_modules/xterm/dist/addons/zmodem/demo/app.js
Caused by this dynamic require which causes webpack to bundle all the addons and their tests:

Perhaps the signature of this.props.addons can be an array of the addon modules directly, rather than an array of strings which are converted into dynamic imports?

farfromrefug commented 6 years ago

@davej i see the webpack issue. I would prefer not to maintain an array. That way whatever plugin are integrated with xterm, it will simply work.

Couldn't you add a rule in webpack to ignore test files from addons? This is what i do when packing for electron

davej commented 6 years ago

I would prefer not to maintain an array.

No, you don't need to maintain a list of plugins, the plugin would be imported outside of the react-xterm lib. Something like this:

import fit from "xterm/dist/addons/fit/fit";
import { XTerm } from "react-xterm";

export default () => (<XTerm addons={[fit]} />);

Couldn't you add a rule in webpack to ignore test files from addons?

I tried the following, but it didn't seem to work (possibly because it's not a direct dependency of my project?):

 externals: [
  "node-pty", "xterm"
]

If I add react-xterm to the array then it works but I end up getting the error seen in #6 instead.

farfromrefug commented 6 years ago

@davej ok i see what you mean. Perso i don't want this. The point of the addon props is not to have to do the import, nor to care about the actual path.

What about this? https://stackoverflow.com/questions/25834396/how-can-i-exclude-code-path-when-bundling-with-webpack-browserify

davej commented 6 years ago

I tried a few similar things as in that link but couldn't get it to work. I ended up basically doing a copy/paste of react-xterm and I'm maintaining it as a local component instead. I'd also prefer not to make xterm an external library if it doesn't need to be.

Feel free to close this if it doesn't fit in with your usage. I can maintain this as a local component, it's not a big problem, 👍.

farfromrefug commented 6 years ago

@davej i don't want you to maintain it as a local component :D This is not the point of open sourcing :D Even if i don't really want to change that prop for now, i want it to work for you

It seems a little bit tricky with webpack but doable: https://stackoverflow.com/questions/25384360/how-to-prevent-moment-js-from-loading-locales-with-webpack/25426019#25426019 https://github.com/webpack/webpack/issues/198#issuecomment-344842525

I would like to help you more, but without knowing the layout of your project or your webpack file, it's a bit tricky

davej commented 6 years ago

My webpack config is:

{
    target: "electron-renderer",
    externals: ["node-pty"]
}

I'm using electron-next. I'll give that webpack plugin a quick try now and see if it works for me.

Grsmto commented 6 years ago

Hi guys, Fyi I had a similar issue and I used the solution you suggested using webpack.IgnorePlugin. Here is my config: new webpack.IgnorePlugin(/\.(test|spec)\./). However I had a context issue in my dynamic import as well! My import statement was like that:

import(`../../pages/${page}`)

and it worked only after adding the file extension like so:

import(`../../pages/${page}.js`)

Hope this help.

lukevp commented 4 years ago

I'm guessing this is no longer an issue in the new RC I am building since xterm no longer bundles addons but will test & close as appropriate.