dougmoscrop / serverless-plugin-include-dependencies

MIT License
184 stars 40 forks source link

[Question] What's the use of utils.isOutside ? #23

Closed hlefebvr closed 5 years ago

hlefebvr commented 6 years ago

Hello there and thanks for the good work !

I'm currently developing a project with the serverless framework and the excellent lerna.js which allows you to create "local" dependencies and share code between modules.

Due to its working logic (creating symbolic links and setting dependency in package.json), using serverless-plugin-include-dependecies failed at first because of utils.isOutside which throwed an error saying that the file was out of the current directory (which is legit). However, when replacing this function's logic with a simple and unconditional return false, I get to deploy my project and reduce (considerably) its serverless.zip size.

Hence my question, what was the original purpose of this function ?

Thanks !

dougmoscrop commented 6 years ago

What *was" happening and may not anymore, was the path of the file gets resolved to it's actually path, not the symlink. Upon deploy, the serverless zip had the source files in the wrong path.

I didn't want the packaging step to quietly break deployment so I added this as a safeguard.

It's worth revisiting for sure just haven't had time.

On Thu, Aug 23, 2018, 8:52 AM Henri Lefebvre notifications@github.com wrote:

Hello there and thanks for the good work !

I'm currently developing a project with the serverless framework and the excellent lerna.js https://github.com/lerna/lerna which allows you to create "local" dependencies and share code between modules.

Due to its working logic (creating symbolic links and setting dependency in package.json), using serverless-plugin-include-dependecies failed at first because of utils.isOutside which throwed an error saying that the file was out of the current directory (which is legit). However, when replacing this function's logic with a simple and unconditional return false, I get to deploy my project and reduce (considerably) its serverless.zip size.

Hence my question, what was the original purpose of this function ?

Thanks !

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dougmoscrop/serverless-plugin-include-dependencies/issues/23, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjzFFk4pmRhlbFQCWR8Gbg9mMdfi_rsks5uTqWKgaJpZM4WJdZO .

hlefebvr commented 6 years ago

Hm, I understand. It does not seem to be the case anymore.

Should I PR with a version with utils.isOutside being removed ? Or else, use environment variable to disable or enable this safeguard ?

dougmoscrop commented 6 years ago

We can remove it but testing is super important. This is a pretty widely used plugin and we don't wanna break things!

Writing integration tests for serverless isn't super easy but it can be done. I would normally do it I'm just super swamped right now, so if you PR with just the code change it's gonna sit for a but before I can get around to testing etc

On Thu, Aug 23, 2018, 10:04 AM Henri Lefebvre notifications@github.com wrote:

Hm, I understand. It does not seem to be the case anymore.

Should I PR with a version with utils.isOutside being removed ? Or else, use environment variable to disable or enable this safeguard ?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/dougmoscrop/serverless-plugin-include-dependencies/issues/23#issuecomment-415425641, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjzFCAzIcn6NpaHg7Cgop8ysUy5xlEVks5uTrZ0gaJpZM4WJdZO .

joscha commented 5 years ago

I got to this issue as well because I have some deliberately placed local dependencies via symlinks and the new version is breaking for me. offline mode still works fine, but deploying now breaks with:

[serverless-plugin-include-dependencies]: Could not find bla

@dougmoscrop what test would you envision to get a new version of this plugin up that supports symlinks? Or we can make it a config flag?

dougmoscrop commented 5 years ago

@joscha does your change in #24 just make your symlink stuff work?

joscha commented 5 years ago

@joscha does your change in #24 just make your symlink stuff work?

As a side-effect, yes - we have some NODE_PATH extensions set up, which are symlinks. There might be some special cases that are not caught by m change potentially.

dougmoscrop commented 5 years ago

Can you try with 3.2.0? I just published it. I tested locally and it worked with a symlinked dependency.

dougmoscrop commented 5 years ago

Has been removed, please re-open if the fix isn't working!