Using local (file) module references in package.json does not work
HyperBrain opened this issue · 10 comments
This is a Bug Report
Description
Using a file reference, e.g. file:../module, does not work and will lead to an error during the packaging.
See #257 for the mention.
Additional Data
- Serverless-Webpack Version you're using: any
The paths are rebased correctly and tested by the new unit test for the functionality in the attached PR.
However I discovered a bug with npm 5 when testing the functionality and created an issue there npm/npm#19183.
I will test, if the bug is restricted to NPM 5 and Windows. If it works with other NPM versions and operating systems, I'll give the fix a go and for NPM5/Win we'll wait for NPM to be fixed then.
@serverless-heaven/serverless-webpack-contributors Does anyone of you have the possibility to check the local file references on OSX with NPM 3 and NPM 5? I will try it on Ubuntu and report the results here.
Ran npm t on fix-npm-file-references branch in OSX
- NPM 5.4.2: unit test passes.
- NPM 3.10.10: unit test passes.
@ceilfors Thanks for the verification 😄
Here are my results for Ubuntu (I created a test project with a local file reference and did serverless package). The expected install output should show the reference.
Serverless: Packing external modules: babel-polyfill@^6.26.0, chalk@file:../chalk, mysql2@^1.4.2, request@^2.82.0`
✔️ NPM 3.10.10
❌ NPM 5.5.1
So, on Ubuntu with NPM 5.5.1 the bug is present. We have to wait until the issue is fixed on NPM side.
@HyperBrain I did not create a test project and ran the unit test on fix-npm-file-references instead. Did the unit test fail in Ubuntu (without creating test project manually)? If it doesn't fail, I think you'll need to revisit the test.
@ceilfors No problem - just saw it.
The unit test passes as it is not an integration test and it verifies the correctness of the created package.json that we use to install the dependencies. The package.json ist fully correct and all file references are rebased to to the webpack output folder correctly. The error only happens with NPM 5 because updating local file references with an existing package-lock file present, is broken there. And we need to copy the package-lock file besides to make sure that only the user's locked dependency versions are installed.
Theoretically we could patch the copied package-lock file and remove the module reference there - this will circumvent the NPM bug. But imo it is quite dangerous to modify the package-lock file because it is an internal mechanism of NPM. The only public data NPM offers is the package.json. If we do so, there is a high risk that it will break with any NPM update - or even lead to disruption of the installation.
The issue is definitely caused by a bug in NPM (NPM 3 works, NPM 5 does not). If we do not copy the package-lock.json, it will work and circumvent the bug at NPM (of course this is not an option, because the locked versions of a project would just be ignored). If they fix the issue on their end, a new 5.5.x version of npm would be released that fixes the package-lock/package behavior. You'll have to update npm in this case to verify that it works with an integration test.
IMO this is even a very critical issue in NPM, because if you copy a local module and just edit the reference in package.json to point to the new location, it will install from the old location.
The same happens even if you work with local references in a project that does not use serverless at all (see the repro steps in the GitHub NPM issue).
In theory we could release the our fix because we do everything right with the refs and create a working output folder, but that would let people open new issues and we'd just tell - use NPM 3 or wait until they fix NPM 5 😃 .
Of course we could add integration tests, but they should be completely separated from any unit tests and they should not be run that frequently. They would be merely for testing if new environmental dependencies like Serverless or NPM still work. Such tests could run serverless package which would show that the integration still works. But I'm afraid that there are so many different variants of a project, that efficient integration testing is nearly impossible (we have different packaging, tens of possible combinations for the content types of the package.json dependencies). Even integration tests would only cover a minimal subset of possible issues caused by bugs in externals (SLS/NPM).
What is your opinion on this?
Took me some time to understand which feature is this issue impacting and finally understood that we have packExternalModules.js.
If NPM is a tight dependency that we have (which seems is), I'd vote for having an integration test for the versions that we would like to officially support. We can have CI matrix if we really want to push for the support of multiple versions of our external dependencies. If there are too many variants and we think that it might be an overkill or we can't support it properly, then maybe we should drop the support for the older versions.
I might be deviating from the original issue here (Apologies as I don't have the full history and context of the feature), but I don't feel that it is right for this plugin to heavily be dependant on package.json or package-lock.json as that's a tight coupling with NPM. Maybe instead of being too smart (executing npm install etc.), we can just copy the node_modules directory from the user. This directory is more universal than NPM. For pruning devDependencies, I'd suggest making it the users' responsibility to npm prune or execute any build ritual before they run sls deploy. Alternatively, maybe we can adopt the functionality of excludeDevDependencies from serverless.
There was already a good discussion, about why we cannot use node_modules and just copy the files from there: #239 . Using NPM (or an other package tool) is the only way to have correctly packages modules.
That's why Serverless' native packaging never worked correctly and that there are lots of problems, regressions and bugs in Serverless in that area. Especially the whole dependency inclusion and exclusion does not really work well there. With webpack it's even harder, because only the compile results of webpack contain which external modules are actually used - this is likely to be very different than the local node_modules tree you have.
I already thought about supporting yarn in the same way as npm and let the user optionally use yarn instead of npm.
I agree that we could have integration tests then, that verify that the plugin still works with specified packaging tools versions (may it be yarn or npm)
Removed the blocked state, because we now have a workaround implemented, thanks to @franciscocpg
@franciscocpg
FYI: Just found out that the related npm bug also affects "npm link" and not only "npm install" with file reference versions. If you have a linked repo and change the version in package.json back to a standard semver version it will just ignore the change and uses package-lock silently (and installs the linked repo).
I will update the npm bug to reflect this information.
This has nothing to do with our PR which is fine, but should put more pressure onto the npm side to fix it 😄
Released with 4.2.0