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

Webpack should have a way to ignore require calls #8826

Closed
sindresorhus opened this issue Feb 24, 2019 · 35 comments
Closed

Webpack should have a way to ignore require calls #8826

sindresorhus opened this issue Feb 24, 2019 · 35 comments

Comments

@sindresorhus
Copy link

sindresorhus commented Feb 24, 2019

Bug report

What is the current behavior?

Webpack tries to handle all require() calls, even those not meant for browser usage. This makes it almost impossible to have a module that works in both Node.js and the browser, but has some added capabilities in Node.js.

Note that this is about authoring reusable npm packages, not end-user apps. I don't have the ability to set any Webpack config like the externals option since I'm making packages other devs will consume.

This is the kind of awfulness we have to do because of Webpack: https://github.com/sindresorhus/ow/blob/d62a06c192b892d504887f0b97fdc842e8cbf862/source/utils/node/require.ts We have wasted countless of hours on this.

Prior discussion that went mostly ignored by the Webpack team: #196 (comment)

What is the expected behavior?

I expected to be able to annotate the code to make Webpack ignore certain require() calls.

For example with a comment:

const require('chalk'); // webpack-ignore

I know for certain that this require() call will not be used in the browser, so I should be able to have it in my source code without Webpack printing warnings to users or erroring out.

I need this as a module maintainer (not user).

Telling users to use the externals config is out of the questions, as my package might be a dependency many levels down.

Some more examples of where I have to use awful code to work around Webpack:

Other relevant information:
webpack version: 4.29.0 (applies to any version though)
Node.js version: 10.15.0
Operating System: macOS 10.14.3
Additional tools:

@SamVerschueren
Copy link

I totally agree, the current solution is ugly and silly. There should be an easier way to work around this problem...

@wmertens
Copy link
Contributor

wmertens commented Mar 4, 2019

I use _non_webpack_require for this. Before I found out about that I had an external named nodeRequire that pointed to the global require

@jhnns
Copy link
Member

jhnns commented Mar 4, 2019

Hi @sindresorhus 👋,

thanks for raising this issue. I agree that it must be annoying for you as a maintainer to get these kind of issues. However, I don't think that webpack is to blame for this either. Dynamic imports is something no bundler is able to handle in a good way. If webpack did not print out a warning for dynamic requires, people might get "Could not find module xyz" on runtime which is worst imo.

I see the magic comment as a last resort because it's webpack specific. I would like to have something that works with all bundlers.

The browser field in the package.json is something that all bundlers respect. So you would have two entries in your module, one that is intended for browsers and one that is intended for node. Then you would require all node-only dependencies in your main entry. Imo electron should add this kind of package switch too because it's less error prone than looking for some magic global variables that are only available in certain environments. Why is that package switch not working for you?

@sindresorhus
Copy link
Author

However, I don't think that webpack is to blame for this either. Dynamic imports is something no bundler is able to handle in a good way.

This is not about dynamic imports.

If webpack did not print out a warning for dynamic requires, people might get "Could not find module xyz" on runtime which is worst imo.

No, they would not get "Could not find module xyz" at runtime because the require call would be behind a conditional that is never hit in the browser. I'm asking for a way to tell Webpack to ignore a require() call because I know best. When automatic behavior fails, we should be able to override it.

I see the magic comment as a last resort because it's webpack specific. I would like to have something that works with all bundlers.

The browser field in package.json is not a standard either. We could come up with a universally recognized magic ignore comment.

The browser field in the package.json is something that all bundlers respect.

Do you have a source for that assertion? Can you point me to the Webpack docs about the browser field?

So you would have two entries in your module, one that is intended for browsers and one that is intended for node. Then you would require all node-only dependencies in your main entry.

You're asking me to massively refactor my codebase to support Webpack. It's not as easy as just using the browser field. It would mean creating entry points that can easily be swapped out. It's not always that simple.

All I want is to make Webpack ignore a require call that is not used in the browser...

@jhnns
Copy link
Member

jhnns commented Mar 5, 2019

This is not about dynamic imports.

That's correct, but dynamic and conditional imports have similar limitations.

No, they would not get "Could not find module xyz" at runtime because the require call would be behind a conditional that is never hit in the browser.

That depends on the quality of the conditional. But I'm sure that you would get it right 😉

Do you have a source for that assertion? Can you point me to the Webpack docs about the browser field?

No, that's just my experience. It's widely adopted in the NPM ecosystem afaict but I have no stats for it. We have no dedicated page for module authors in our docs, but if you're looking for the issue number: #151

You're asking me to massively refactor my codebase to support Webpack. It's not as easy as just using the browser field. It would mean creating entry points that can easily be swapped out. It's not always that simple.

That's true: it's not an easy refactoring. But again, it's not only to support webpack. Other bundlers might just have decided to ignore occurrences of require() they don't understand, which is not ideal as well.

While I personally think that every code base would benefit from this refactoring, I do understand that it's not feasible for module authors with existing code. So I see the use case. How would that comment look like?

require(/* ignore */ "./something.js")

This would be similar to:

import(/* webpackChunkName: "something" */ "./something.js")

I guess the ignore comment would only work for CommonJS require() and dynamic import().

Would do other bundler maintainers think about this? @lukastaegert @devongovett @goto-bus-stop

@goto-bus-stop
Copy link
Contributor

goto-bus-stop commented Mar 5, 2019

For the "browser" field, most popular bundlers support excluding dependencies:

"browser": {
  "chalk": false
}

AFAIK, browserify, webpack and rollup all support this. If Parcel uses browser-resolve it supports it too (not sure). Meteor doesn't support it, last I checked.

Browserify intentionally ignores dynamic requires entirely, so you can do require('ch' + 'alk'). I've seen this used in cryptography related modules in the browserify ecosystem, sometimes they'll do require('cr'+'ypto') in node and fall back to browser APIs in the browser. Browserify also doesn't track aliasing so you can do nodeRequire = require; nodeRequire('chalk'). For webpack, ignoring dynamic requires is probably not an option… I don't know of any use case for aliasing though aside from this, so that might be on the table?

But ideally, the browser field object form (w|c|sh)ould be used for this :)

@lukastaegert
Copy link

The browser field seems well documented to me: https://github.com/defunctzombie/package-browser-field-spec

Personally, I am not a fan of introducing a new universal annotation when we have something that is already widely adopted. The browser field also supports replacing or ignoring very specific imports. For rollup, this is solved by providing a dummy file with an empty object as default export in those cases.

Otherwise as we do not natively support dynamic require but only dynamic imports, those are handled rather permissively: If they cannot be resolved at runtime they will be left in the code unchanged, which should not be an issue if the code is never run. I plan to at least print a proper warning when a dynamic import that is just a string cannot be resolved, similar to how we handle unresolved static imports (a warning telling you that we assume this is external). However, I would only print this warning in case the dynamic import is not tree-shaken anyway. To me it seems that at least some parts of this should be solvable via tree-shaking.

@sindresorhus
Copy link
Author

For anyone else encountering this issue and need a workaround, this is probably the best one: https://twitter.com/kamilogorek/status/1102272038411137025

@TheLarkInn
Copy link
Member

TheLarkInn commented May 9, 2019

@sindresorhus There's no doubt your contributions to the NodeJS/Isomorphic JS ecosystem is considerable. And we are sorry its been a burden so far.

The more bundle-able modules are the more positive impact to your modules consumers. So its really important to us, to help you make this easier!

You have the maintainers of Rollup (@lukastaegert), Browserify (@goto-bus-stop), and webpack here all verbally in support of the standard that exists today with the browser field.

Is there maybe a hotlist of "most requested" modules you own that would need considerable refactoring? As far as I'm concerned we could all try and pitch our respective contributor ecosystems to help PR this code for you! (If you are willing).

We're in it together <3 🤗

@devongovett
Copy link

In parcel at least, you can also do this:

if (process.browser) {
  require('./browser');
} else {
  require('./node');
}

Not sure about other bundlers.

@TheLarkInn
Copy link
Member

TheLarkInn commented May 10, 2019

We can do similar, but does this error in Browser ENV for non bundler using consumers? That's my hypothesis why it could not be the most feasible for some library authors.

@lukastaegert
Copy link

It will also error when bundled vanilla Rollup as we do not polyfill globals by default (which is partly because otherwise we would mess up those patterns when bundling for libraries, though).

@TheLarkInn
Copy link
Member

We do like leveraging process however this may cause optimization bailouts for our users inside deep module graphs because we do mock injections on process standalone. We do leverage process.env.NODE_ENV but looks like @lukastaegert mentioned it is problematic still.

@devongovett
Copy link

Parcel inlines process.browser similar to how we inline process.env.NODE_ENV. When collecting dependencies, we check if they are inside a falsy branch, and ignore if so (leaving require as is). This applies to any branch, including process.env comparisons and process.browser checks, but also any other code we can evaluate statically. Since process.browser is inlined, we don't include the process runtime at all unless other process features are used.

@TheLarkInn
Copy link
Member

@devongovett That's interesting, perhaps when we use the target feature, maybe we should also have a process.target or process.env.target inlining. I guess it doesn't solve the "people of non bundler" use cases though right (in the browser scenario?).

@devongovett
Copy link

Yeah non-bundler people would be a problem, but if you're using require it wouldn't work in a browser anyway right?

@TheLarkInn
Copy link
Member

TheLarkInn commented May 10, 2019

@devongovett Haha I told myself the same thing, but a great example is at Microsoft we have some legacy teams using requireJS, and therefore still can consume require's yet get that kind of process.whatever code and it nukes.

Not a deal-breaker for the majority, but a consideration.

Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this issue Dec 19, 2019
…isplay/api.js`

For performance reasons, and to avoid hanging the browser UI, the PDF.js library should *always* be used with web workers enabled.
At this point in time all of the supported browsers should have proper worker support, and Node.js is thus the only environment where workers aren't supported. Hence it no longer seems relevant/necessary to provide, by default, fake worker loaders for various JS builders/bundlers/frameworks in the PDF.js code itself.[1]

In order to simplify things, the fake worker loader code is thus simplified to now *only* support Node.js usage respectively "normal" browser usage out-of-the-box.[2]

*Please note:* The officially intended way of using the PDF.js library is with workers enabled, which can be done by setting `GlobalWorkerOptions.workerSrc`, `GlobalWorkerOptions.workerPort`, or manually providing a `PDFWorker` instance when calling `getDocument`.

---
[1] Note that it's still possible to *manually* disable workers, simply my manually loading the built `pdf.worker.js` file into the (current) global scope, however this's mostly intended for testing/debugging purposes.

[2] Unfortunately some bundlers such as Webpack, when used with third-party deployments of the PDF.js library, will start to print `Critical dependency: ...` warnings when run against the built `pdf.js` file from this patch. The reason is that despite the `require` calls being protected by *runtime* `isNodeJS` checks, it's not possible to simply tell Webpack to just ignore the `require`; please note [Webpack issue 8826](webpack/webpack#8826) and libraries such as [require-fool-webpack](https://github.com/sindresorhus/require-fool-webpack).
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this issue Dec 19, 2019
…isplay/api.js`

For performance reasons, and to avoid hanging the browser UI, the PDF.js library should *always* be used with web workers enabled.
At this point in time all of the supported browsers should have proper worker support, and Node.js is thus the only environment where workers aren't supported. Hence it no longer seems relevant/necessary to provide, by default, fake worker loaders for various JS builders/bundlers/frameworks in the PDF.js code itself.[1]

In order to simplify things, the fake worker loader code is thus simplified to now *only* support Node.js usage respectively "normal" browser usage out-of-the-box.[2]

*Please note:* The officially intended way of using the PDF.js library is with workers enabled, which can be done by setting `GlobalWorkerOptions.workerSrc`, `GlobalWorkerOptions.workerPort`, or manually providing a `PDFWorker` instance when calling `getDocument`.

---
[1] Note that it's still possible to *manually* disable workers, simply my manually loading the built `pdf.worker.js` file into the (current) global scope, however this's mostly intended for testing/debugging purposes.

[2] Unfortunately some bundlers such as Webpack, when used with third-party deployments of the PDF.js library, will start to print `Critical dependency: ...` warnings when run against the built `pdf.js` file from this patch. The reason is that despite the `require` calls being protected by *runtime* `isNodeJS` checks, it's not possible to simply tell Webpack to just ignore the `require`; please see [Webpack issue 8826](webpack/webpack#8826) and libraries such as [require-fool-webpack](https://github.com/sindresorhus/require-fool-webpack).
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this issue Dec 19, 2019
… dependency: ...` warnings from Webpack

Since bundlers, such as Webpack, cannot be forced to leave `require` statements alone we are thus forced to jump through hoops in order to prevent these warnings in third-party deployments of the PDF.js library; please see [Webpack issue 8826](webpack/webpack#8826) and libraries such as [require-fool-webpack](https://github.com/sindresorhus/require-fool-webpack).

*Please note:* This is based on the assumption that code running in Node.js won't ever be affected by e.g. Content Security Policies that prevent use of `eval`. If that ever occurs, we should revert to a normal `require` statement and simply document the Webpack warnings instead.
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this issue Dec 19, 2019
… dependency: ...` warnings from Webpack

Since bundlers, such as Webpack, cannot be told to leave `require` statements alone we are thus forced to jump through hoops in order to prevent these warnings in third-party deployments of the PDF.js library; please see [Webpack issue 8826](webpack/webpack#8826) and libraries such as [require-fool-webpack](https://github.com/sindresorhus/require-fool-webpack).

*Please note:* This is based on the assumption that code running in Node.js won't ever be affected by e.g. Content Security Policies that prevent use of `eval`. If that ever occurs, we should revert to a normal `require` statement and simply document the Webpack warnings instead.
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this issue Dec 19, 2019
… dependency: ...` warnings from Webpack

Since bundlers, such as Webpack, cannot be told to leave `require` statements alone we are thus forced to jump through hoops in order to prevent these warnings in third-party deployments of the PDF.js library; please see [Webpack issue 8826](webpack/webpack#8826) and libraries such as [require-fool-webpack](https://github.com/sindresorhus/require-fool-webpack).

*Please note:* This is based on the assumption that code running in Node.js won't ever be affected by e.g. Content Security Policies that prevent use of `eval`. If that ever occurs, we should revert to a normal `require` statement and simply document the Webpack warnings instead.
@sokra
Copy link
Member

sokra commented Aug 10, 2020

To close up this issue:

Do not try to workaround webpacks parser to create node-only require()s!

Do use the browser field in package.json to differ between environments. See also @jhnns comment about that: #8826 (comment)

In future you may also use the exports field in package.json.

Creating parser hacks has multiple disadvantages:

  • It may break when the parser becomes smarter
  • I breaks bundling for node.js usage
  • It includes unneeded code for browser usage
  • Future tools have to emulate the way this stuff is parsed to be compatible.
    • We also had to add hacks to support hacks people did for browserify

A webpack ignore has also multiple disadvantages:

  • It's webpack-specific and breaks other bundlers (or they have to support webpack ignore too, which would be weird)
  • I breaks bundling for node.js usage
  • It breaks when minimized and comments are removed

A package.json field has these advantages:

  • It's supported by all bundlers
  • It still allows to bundle for node.js
  • It's statically analysable
  • The code doesn't become weird
  • It's more future-proof

@sokra sokra closed this as completed Aug 10, 2020
@wmertens
Copy link
Contributor

@sokra but how to require something at runtime in a bundle that was created by webpack?

@alexander-akait
Copy link
Member

@wmertens You can bundle something by webpack/other tool, and use externals

@msklvsk
Copy link

msklvsk commented Feb 7, 2021

What about this use case?

import Config from '../config' // use a sample/dev config for free typings

function main() {
  const config = require(process.argv[2]) as typeof Config
  // your `config` is typed
  // ...
}

externals can’t handle this. (Nothing browser here, node-only.)

@dvcrn
Copy link

dvcrn commented Mar 25, 2021

So what was the conclusion here? To use browser in the package.json?

I am still running against a wall with third-party node modules that have a

if (!inBrowser) {
  foo = require("bar");
}

in them, which give me a "module not found" error on build. I added the browser field to my package.json as well:

  "browser": {
    "bar": false,
  },

So how to solve this? I could fork all the modules that have this and manually patch them but it would be much easier if I could just dummy-out this require because it would never get hit due to the conditional if

@sokra
Copy link
Member

sokra commented Mar 25, 2021

You can also do the same in config

resolve.alias.bar: false

You can do that globally or inside of module.rules to only affect certain modules

@dvcrn
Copy link

dvcrn commented Mar 25, 2021

@sokra thanks for the quick reply! I tried that method already but without luck - I'm still getting the "Module not found" errors (it also complained when I tried to set it to false that the value has to be of type string for aliases).

Using create-react-app with craco here to modify the webpack config.

Another thing I tried without luck was setting "paths" in package.json.

The modules I'm trying to nop-out is crypto

@sokra
Copy link
Member

sokra commented Mar 25, 2021

Seems like you are using webpack 4 and that's only available for webpack 5.

Try this instead: externals: { crypto: "var {}" }

@dvcrn
Copy link

dvcrn commented Mar 25, 2021

Yes you're right, create-react-app still uses webpack 4.

Using externals in the webpack config solved it for me, it's building now! Thanks for the quick help!

@JounQin
Copy link
Member

JounQin commented May 14, 2021

What about this use case?

import Config from '../config' // use a sample/dev config for free typings

function main() {
  const config = require(process.argv[2]) as typeof Config
  // your `config` is typed
  // ...
}

externals can’t handle this. (Nothing browser here, node-only.)

@msklvsk If you just want webpack bundles correctly, you can use __non_webpack_require__, if you want it also works from source, there is a hack pattern from prettier: eval('require')('your-dep')(You'd better replace this hack back after compiling).

@ravimaharjan
Copy link

ravimaharjan commented May 20, 2021

https://www.npmjs.com/package/webpack-ignore-dynamic-require
Check this package. Webpack ignores the dynamic require with this.

@hkjpotato
Copy link

Yes you're right, create-react-app still uses webpack 4.

Using externals in the webpack config solved it for me, it's building now! Thanks for the quick help!

@dvcrn can you share how you add "externals" in create-react-app? It seems no easy way to modify the webpack configs.

@dvcrn
Copy link

dvcrn commented Jun 10, 2021

@hkjpotato I'm not at my machine right now so I can't check the full code but we're using craco to override webpack configuration. Something like this in craco config (copied from a different github issue):

module.exports = {
  webpack: {
    configure: (webpackConfig, { env, paths }) => {
      webpackConfig.externals = {
        react: 'React',
        'react-dom': 'ReactDOM'
      }
      return webpackConfig;
    }
  }
}

@Vector-Green
Copy link

How to ignore whole directory from require bundled by webpack?)

@rdsedmundo
Copy link

rdsedmundo commented Jan 18, 2024

In need of this 5 years on. Coincidentally it was for the chalk module as well, which I used only during tests. I don't get why it's possible to do for a dynamic import call with /* webpackIgnore: true */ but not with require.

@alexander-akait
Copy link
Member

@rdsedmundo Due perf reasons, but it works if you set https://webpack.js.org/configuration/module/#moduleparserjavascriptcommonjsmagiccomments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests