Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.Sign up
Using local (file) module references in package.json does not work #263
The paths are rebased correctly and tested by the new unit test for the functionality in the attached PR.
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.
@ceilfors Thanks for the verification
So, on Ubuntu with NPM 5.5.1 the bug is present. We have to wait until the issue is fixed on NPM side.
@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
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
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
What is your opinion on this?
Took me some time to understand which feature is this issue impacting and finally understood that we have
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
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.
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)
This has nothing to do with our PR which is fine, but should put more pressure onto the npm side to fix it