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

Uncaught ReferenceError: process is not defined #38

Closed
ericsoco opened this issue Sep 17, 2019 · 12 comments
Closed

Uncaught ReferenceError: process is not defined #38

ericsoco opened this issue Sep 17, 2019 · 12 comments
Labels
🙋 no/question This does not need any changes 🙅 no/wontfix This is not (enough of) an issue for this project

Comments

@ericsoco
Copy link

ericsoco commented Sep 17, 2019

Subject of the issue

The VFile constructor references process directly. In a vanilla browser environment, this throws an error. However, many build systems polyfill process so this may have gone unnoticed in most applications -- process is mocked by default by Webpack, and Browserify does something similar.

Your environment

  • OS: Mac OSX 10.14.5
  • Packages: unified@6.2.0, vfile@2.3.0 (via react-markdown@4.2.2)
  • Env:
    • node@10.16.0
    • npm@6.9.0
    • yarn@1.16.0
    • Chrome v76.0.3809.132

Steps to reproduce

In an environment without a global process object (e.g. a vanilla browser, a webpack build with the process mock disabled):

Expected behaviour

The VFile() constructor instantiates an object.

Actual behaviour

The VFile() constructor throws.

@wooorm
Copy link
Member

wooorm commented Sep 18, 2019

Correct, vfile depends on process, so the mocking of process in other environments is indeed expected. I see it as expected behaviour, not as an issue 🤷‍♂️

@ericsoco
Copy link
Author

vfile claims to work in the browser. From the README:

Plus, they work in the browser.

It does work in the browser, but only if process (specifically, process.cwd()) is polyfilled. This is a non-obvious gotcha.

How is VFile.cwd used by vfile consumers? Browsing the source (via GH search, which can yield spotty results), I don't see it referenced in either vfile or unified...

@wooorm
Copy link
Member

wooorm commented Sep 18, 2019

Btw, we talked about this with rexxars from react-markdown already: #16 (comment)

It does work in the browser, but only if process (specifically, process.cwd()) is polyfilled. This is a non-obvious gotcha.

This is indeed how it is intended: it depends on certain aspects, which are available in Node, and typically available in bundlers as well. Unless explicitly turned off.

How is VFile.cwd used by vfile consumers? Browsing the source (via GH search, which can yield spotty results), I don't see it referenced in either vfile or unified...

file.cwd is used because file.path can be relative, in which case, it should be read relative to file.cwd. Indeed, GH search isn’t great, but searching across the collective yields more results.


There are several downsides for touching/changing/removing cwd, as outlined in the issue mentioned above, but also: It’s breaking. vfile itself was downloaded 100m times this year. In total, the projects in the collective are downloaded more than 2B times a year. These numbers are multiplying manyfold per year. Breaking stuff that’s used so much has consequences.

@ericsoco
Copy link
Author

ericsoco commented Sep 18, 2019

Understood. Thanks for the context. For our project, I've restored the Webpack process polyfill that our framework removes. Probably safe to close this as a wontfix given the risk involved in a change, given the change will likely benefit a much smaller percentage of users than those negatively impacted by an error in a change.

@wooorm
Copy link
Member

wooorm commented Oct 3, 2019

Sweet, thanks for raising this and for understanding!

Best

@wooorm wooorm closed this as completed Oct 3, 2019
@wooorm wooorm added 🙋 no/question This does not need any changes 🙅 no/wontfix This is not (enough of) an issue for this project and removed 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Oct 3, 2019
@ericsoco
Copy link
Author

ericsoco commented Oct 9, 2019

Heads up that Webpack is considering removing default polyfilling, so this may become more of an issue moving forward.

You might consider leaving them feedback if you have thoughts (they're asking for feedback on the decision).

@wooorm
Copy link
Member

wooorm commented Oct 10, 2019

Ah thanks for mentioning that! I found an issue where other module authors already voiced my concerns. I’m interested in seeing what the actual behaviour will be: when warnings show up, we’ll address them somehow!

@IdeaHunter
Copy link

@wooorm
FIY - current webpack 5.0.2-beta behaviour is following:

ERROR in ./node_modules/unified/node_modules/vfile/core.js 3:11-26
Module not found: Error: Can't resolve 'path' in 'c:\Frontend\node_modules\unified\node_modules\vfile'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need these module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add an alias 'resolve.alias: { "path": "path-browserify" }'
        - install 'path-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.alias: { "path": false }
 @ ./node_modules/unified/node_modules/vfile/index.js 4:12-32
 @ ./node_modules/unified/index.js 6:12-28
 @ ./node_modules/react-markdown/lib/react-markdown.js 13:14-32
 @ ./src/vendor.js 38:0-23
 @ dll vendor vendor[0] 

@Nimphal
Copy link

Nimphal commented Jun 8, 2020

In case anyone comes across this after upgrading to webpack 5, the way to shim process in the browser (following instructions from here https://webpack.js.org/guides/shimming/ ):
package.json

  "devDependencies": {
   ...
    "process": "0.11.10",
  }

webpack.config.json

module.exports = {
  ...
  plugins: [
      new webpack.ProvidePlugin({
             process: 'process/browser',
      }),
  ],
}

@govizlora
Copy link

govizlora commented Aug 29, 2020

ProvidePlugin works, but it shims process to all the modules. I've seen some modules using the existence of process to test if the environment is browser or node, in this case we might prefer not providing process to those modules.

To only shim vfile, you can use imports-loader instead, as introduced here: https://webpack.js.org/guides/shimming/#granular-shimming. (You still need to have process in your package.json)

module.exports = {
  module: {
    rules: [
      {
        test: /node_modules\/vfile\/core\.js/,
        use: [{
          loader: 'imports-loader',
          options: {
            type: 'commonjs',
            imports: ['single process/browser process'],
          },
        }],
      },
    ],
  },
};

@facuescobar

This comment has been minimized.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Nov 12, 2020

the question around resolving process is answered in #38 (comment), specific webpack configs that may work are offered in #38 (comment) and #38 (comment)
As the issue tracker is used as a backlog for bugs/feature requests, I'm locking this issue as resolved.
Follow up questions are welcome in the discussion/Q&A board https://github.com/vfile/vfile/discussions, but are not a good fit for the issue tracker.

@vfile vfile locked as resolved and limited conversation to collaborators Nov 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🙋 no/question This does not need any changes 🙅 no/wontfix This is not (enough of) an issue for this project
Development

No branches or pull requests

7 participants