jshttp / content-disposition

Create and parse HTTP Content-Disposition header
MIT License
224 stars 43 forks source link

Doesn't work anymore if path package is installed #37

Closed kud closed 3 years ago

kud commented 3 years ago

Hello,

Since I've installed backstopjs, I can't use anymore content-disposition correctly, as backstop installs the path package and content-disposition also uses it but without installing it.

Maybe this package was done only for node but it worked well also for as web oriented. But if you install path, it's broken.

cf https://github.com/garris/BackstopJS/issues/1351

Do you have any idea how to fix that? 😊

This question is legit only if you think your lib should also work for web, not only for node.

Thanks!

dougwilson commented 3 years ago

Hi @kud I'm sorry you are having trouble. I am not familiar with what BackstopJS is, unfortunately. This module was created for Node.js, and uses a lot of the Node.js built-ins (like path), and in Node.js, you cannot add path to the package.json, otherwise it will cause the path module from npm to be installed, which is not desired in a Node.js package.

kud commented 3 years ago

@dougwilson Yes! I totally get it. I'll certainly submit a PR saying it's for node only.

Thank you for your time. :)

dougwilson commented 3 years ago

So just to see, I installed the path modules and this module still works with it, though it looks like the npm path module is a copy of the bulit-in version from Node.js 0.12.7, so quite old. I looked at your issue, and the issue seems to be unrelated to this module cannot use the npm path module, but rather with the npm path module itself, as the error you are getting is that process is not defined in that npm path module.

kud commented 3 years ago

Crystal explanation! <3

dougwilson commented 3 years ago

I'll certainly submit a PR saying it's for node only.

Are you sure that is the case? I webpack this module for use on the frontend without any issues. It seems in your BackstopJS issue that your issue is with the npm path module, which this module is not requiring, so it seems there is something more going on. Adding a misleading statement to the readme here is probably not the right action :)

kud commented 3 years ago

I'll certainly submit a PR saying it's for node only.

Are you sure that is the case? I webpack this module for use on the frontend without any issues. It seems in your BackstopJS issue that your issue is with the npm path module, which this module is not requiring, so it seems there is something more going on. Adding a misleading statement to the readme here is probably not the right action :)

True that!

Sorry for the answers, it's not as easy as a chat :D I had your answer just after what I said. 😊

dougwilson commented 3 years ago

Probably the only reason you are seeing this module named in your error is because this one just happened to be the first to require('path') in your stack.

Edit: posted this as you posted. It's no problem; I am happy to help out however I can.

kud commented 3 years ago

Adding this to my webpack made it work.

// webpack needs to be explicitly required
const webpack = require('webpack')

module.exports = {

/* ... rest of the config here ... */

  plugins: [
    // fix "process is not defined" error:
    // (do "npm install process" before running the build)
    new webpack.ProvidePlugin({
      process: 'process/browser',
    }),
  ]
}

It's because process wasn't defined (as it's required in the npm module path like process.platform === "win32"), now it is. :)