Compile pluginsLocalPaths array for bundle exclusion

[fredericbarthelet]

Closes: #7908

[codecov-commenter]

Codecov Report

Merging #7931 into master will decrease coverage by 0.02%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #7931      +/-   ##
==========================================
- Coverage   88.30%   88.27%   -0.03%     
==========================================
  Files         244      245       +1     
  Lines        9082     9095      +13     
==========================================
+ Hits         8020     8029       +9     
- Misses       1062     1066       +4     

Impacted Files	Coverage Δ
lib/plugins/aws/info/getStackInfo.js	87.75% <57.14%> (-0.62%)	⬇️
lib/plugins/aws/utils/resolveCfImportValue.js	57.14% <57.14%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ffa549...9016181. Read the comment docs.

[medikoo]

Great thanks @fredericbarthelet for taking that. Solution looks very promising!

Still, please see my comment I believe we need to imply more intelligent handling to plugins referenced with relative paths.

[medikoo]

This collection may contain both directory and file paths. e.g. when local plugin referenced as ./plugin, may actually point ./plugin.js. Therefore treating all paths as directories won't work well.

It's hard to have 100% bulletproof solution for that, but I think what could work best is:

• Agree that getLocalPluginsPaths may return paths to directories, and files. To make distinction easy, let's ensure that all directory paths are postfixed with / (or system path separator - we need to be careful with Windows here)
• In getLocalPluginsPaths return paths as follows
  • If there's a custom local path, return it with postfixed / (as it's clearly a directory path)
  • Resolve final paths to plugins referenced with relative paths (starting with ./), as it's done here:

    serverless/lib/classes/PluginManager.js

    Line 26 in 5a444c4

    return require(cjsResolve(servicePath, pluginPath).realPath);

    . If path as input into configuration, points a directory (in which main plugin module lives) then return it postfixed with /, if path points a plugin file, then return it with fully resolved file extension.

Having above ensure glob rules are applied differently to directories and files

[fredericbarthelet]

Thank you for your review @medikoo

I understand your proposal, however IMHO it won't also be 100% bulletproof.

Consider the following plugin files :

|_ serverlessProjectRoot
    |_ serverless.yml
    |_ pluginDirectory
        |_ mySuperPlugin.js
        |_ utils.js

Consider as well the following configuration for this projet serverless service file :

plugins: ['./pluginDirectory/mySuperPlugin']

Using your proposal, we would in fact identify that mySuperPlugin should be resolved to ./pluginDirectory/mySuperPlugin.js instead of ./pluginDirectory/mySuperPlugin/ and therefore exclude this file rather than pluginDireectory/mySuperPlugin/** glob.
However, if this file locally requires utils.js, it will not be identified as a file to be excluded and will be shipped with lambda package. Correct ?

Achieving the same result, we could simply generate 2 exclusion blobs from the getLocalPluginsPaths method call : for each item of this list, one exact file resolution blob ${item}.js and one directory resultion blob ${item}/**. This would not require module resolution from the packageService.

With this solution and the same architecture from above, the exclusion list will include pluginDirectory/mySuperPlugin.js and pluginDirectory/mySuperPlugin/**, resulting in the same quantity of excluded files.

WDYT ?

[medikoo]

However, if this file locally requires utils.js, it will not be identified as a file to be excluded and will be shipped with lambda package. Correct ?

Yes, agreed, it's exactly the reason I mentioned it's hard to have 100% bulletproof solution :)

for each item of this list, one exact file resolution blob ${item}.js and one directory resultion blob ${item}/**

This may work for most of the cases, still is immune to one flaw. Imagine user having both foo.js and foo folder which is expected to land in lambda. With above we will not package foo folder, and that will be clearly a bug on our side.

Of course it's an edge case, but one we can avoid by resolving the main plugin module path, and reasoning against that. It'll be nice to not open door for false positives.