Local packages and component downloads

Pavel910 commented 5 years ago

@eahefnawy I'd like to suggest/discuss an alternative way of handling component downloads.

The main problem for me is that Template throws an error if it doesn't find the component package on NPM. However, my component packages are still far from ready for publishing to NPM, and they are located in my monorepo with all the other packages, and are resolvable via node_modules.

I have 2 possible solutions: 1) when a component is not found on NPM, but can be resolved via require.resolve - just use that and do not throw an error. 2) add a configuration flag to component, something like this:

api:
  component: '@webiny/microservice'
  registry: false // <-- this would skip all registry checks

I personally prefer the first solution as it does not require any additional code/configuration on developer's side.

Currently the only packages that are not downloaded are the ones starting with ., which is not very useful, as I want to use the component name as a regular npm module, like @webiny/microservice, but it is a local/private package that is not on npm.

Looking forward to hearing your opinion on this! 🍻

EDIT: Here is the modification required to implement the first solution.

https://github.com/serverless/core/blob/master/src/utils/download.js#L82

let componentVersionToInstall
try {
    componentVersionToInstall = await getComponentVersionToDownload(component)
} catch (e) {
    if (require.resolve(component)) {
        componentsPathsMap[component] = component
        return
    }
    throw e
}

eahefnawy commented 5 years ago

Thanks for opening this up @Pavel910 ... curious, why do you dislike local paths ../microservice? 🤔

That extra registry key is interesting. We're considering having alternative registry options other than than npm so it might prove helpful here. But yeah, for our purpose here, I like the first option more.

Doesn't seem to be a big change, so PR is welcome 😊... I'd just appreciate a comment on that line explaining why we have it here, cause this is an interesting use case that I did not think of, and will probably forget! 😅

Pavel910 commented 5 years ago

@eahefnawy there are several reasons why ../../ is not very practical: 1) I'm working on a PR for aws-cloudfront and I forked and cloned the repo locally, added my changes, but now I want to test it on a real-life project I have in a different repo, so to reference the aws-cloudfront component I used an absolute path /Users/pavel/webiny/js/aws-cloudfront - which fails because it starts with /, and SLS attempts to look for it on npm.

2) I have a yarn monorepo, meaning all my workspaces are linked in the main node_modules, so I can use the actual package name, for example @webiny/serverless-api, anywhere - except in serverless.yml, and since I have many components and several sub-projects with their own serverless.yml files, I have a lot of ../../ walking to do, to reach the actual component location.

For the time being I did this:

components:
  function: '../../../node_modules/@webiny/serverless-function'
  api: '../../../node_modules/@webiny/serverless-api'

site:
  component: ${components.function}

You can see how awkward it is, and imagine changing the folder structure (which I do regularly while experimenting with project organization)...

I'll send a PR, it's a small change and doesn't change the current behavior, just gives more freedom with not-yet-published packages.

eahefnawy commented 5 years ago

That makes sense @Pavel910 ... Just left a comment in your PR. But other than that, this change is welcome.

P.S: We really appreciate you always opening up an issue discussing the change before PRing. Thanks for that 🙌

serverless / template

Local packages and component downloads #6