rxdi / firelink

Firebase. gcloud and monorepos are not combining very well until they met @rxdi/firelink
MIT License
68 stars 9 forks source link

add package naming convention #3

Closed almeynman closed 4 years ago

almeynman commented 4 years ago

Hey, awesome work!

I had encountered an issue when trying to use the package, the dependencies had undefined in their package name. After debugging for a bit I noticed that you rely on @group/package naming convention. Decided to add it to the README, so that others would not spend time on it

Stradivario commented 4 years ago

Thank you very much for what you have created! You are one of the first contributing to @rxdi infrastructure :)))

Sounds like a missing feature to support packages without namespace.

https://github.com/rxdi/firelink/blob/master/src/helpers/modify-json.ts#L12

I think this is the line which needs to be modified. Will try in the end of the day to think about solution for both scenarios.

Also i am thinking how we can use it with single naming convention like Lerna monorepo tool uses. And maybe we need to throw error if name is not present ?

What do you think @almeynman ?

Also naming convention introducing your name like a branch is really cool! It can help us to track multiple contributors easily :))

almeynman commented 4 years ago

@Stradivario Hey, thanks for pointing to the right line. I think this can be fixed quite easily then like so:

    dependencies.map(async ({ dep }) => {
      packageJson.dependencies[dep] = `file:./${linkedPackagesName}/${
        dep.includes('/') ? dep.split('/')[1] : dep
      }`;
    })

What do you think? if it's ok, then I can make a PR

Stradivario commented 4 years ago

@almeynman Sounds perfect for me please feel free to add it to codebase ! :)

I have made some non braking changes with this pull request https://github.com/rxdi/firelink/pull/5

Put some CI/CD using githubactions, some tests, added prettier, created enum for some parts of the logic.

Before you start maybe you want to rebase or merge existing logic from master since there are some lint rules and tests to pass.

Thank you very much for your effort!

Regards, Kristiyan Tachev

almeynman commented 4 years ago

Closing this one, as the other PR fixes the issue