Skip to content
This repository has been archived by the owner on May 1, 2020. It is now read-only.

Local packages and component downloads #6

Open
Pavel910 opened this issue Sep 17, 2019 · 3 comments
Open

Local packages and component downloads #6

Pavel910 opened this issue Sep 17, 2019 · 3 comments

Comments

@Pavel910
Copy link
Contributor

Pavel910 commented Sep 17, 2019

@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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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 🙌

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants