pillarjs / send

Streaming static file server with Range and conditional-GET support
MIT License
796 stars 188 forks source link

check mime Version, use correct mime Function #172

Closed spacecowboy23 closed 5 years ago

spacecowboy23 commented 5 years ago

Polymer 3 uses Yarn as Package-Manager and needs the --flat option set.

send has mime as dependency and there are some breaking changes between mime v1 and mime v2.

With the need for the yarn --flat option in Polymer 3, only one Version of mime can be installed for the project. So, if i have mime v2 installed (remember,yarn --flat allows only one Version per package), send calls mime.lookup and mime.charset wich are not available in mime v2.

The commit tests wich function is available in send (send v1 uses lookup(), send v2 uses getType()) and uses the recognized function.

dougwilson commented 5 years ago

Sorry, that was a comment to a different issue.

dougwilson commented 5 years ago

Thanks for this. I've never heard of this before. This kind of seems like a weird restriction -- npm from my understanding allows us to declare the version of modules we need, for this very reason. I'm not sure doing this is really a sustainable way to move forward, though.

At the very least, we need tests added for the new logic branching added and documentation added about this, especially if this means the mime types this module returns will no longer be predictable (i.e. because we can no longer control them by the version of mime we depend on).

dougwilson commented 5 years ago

The mime library is directly part of our public API as require('send').mime, so swapping it out may break other modules that use send, especially if they are defining their own mime types. This issue and caveats likely needs to at least be documented in the README if this is something that is supported 👍

spacecowboy23 commented 5 years ago

I will add tests. Yes, npm will allow to install the module we need, but with yarn --flat (never used yarn before) you are able to install only version per package. From the yarn docs:

Install all the dependencies, but only allow one version for each package. On the first run this will prompt you to choose a single version for each package that is depended on at multiple version ranges. These will be added to your package.json under a resolutions field.
dougwilson commented 5 years ago

Gotcha. Yea, never tested with yarn and even at the top of the readme for this module it is explicitly stated it is for npm. We probably want to test that too.

dougwilson commented 5 years ago

Also that polymer link you included seems to say you shouldn't be using flat with server side npm dependencies at all, which is probably why you're the first one ever reporting this? Is anyone else even havig this issue following that polymer setup instructions?

spacecowboy23 commented 5 years ago

The Problem we encountered is in our case webpack-dev-server / webpack-dev-middleware. Webpack relies on mime v2. Imho the Problem is that you can't really mix npm and yarn. There is no option or flag in yarn that package.json should be ignored (afaik). See [https://github.com/yarnpkg/yarn/issues/2550]

Sure, you can install package 'foo' with npm and package 'bar' with yarn, but each time you call yarn add bar, yarn also installs/parses all packages from package.json. Otherwise i set up my project the wrong way - would be happy if this is the reason

dougwilson commented 5 years ago

So the link you provided has the following section:

If you have other npm dependencies, such as servers, dev tools, or compilers, installing all dependencies flat may cause version conflicts. In that case, you have several options:

Did any of those solutions work for you? It sounds like it describes your exact issue.

dougwilson commented 5 years ago

Hi @spacecowboy23 so what is the status on this pull request? I don't see any changes requested made, and never got an answer to my question in the previous comment.

dougwilson commented 5 years ago

Closing due to no response.