Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monorepo #200

Merged
merged 17 commits into from
Dec 6, 2022
Merged

Monorepo #200

merged 17 commits into from
Dec 6, 2022

Conversation

0xjei
Copy link
Contributor

@0xjei 0xjei commented Nov 17, 2022

This monorepo approach where the CLI commands have a dedicated package of actions and helpers, the backend is organized in a more generic fashion will ease the burden of testing and maintainability.

BREAKING CHANGE: The folder structure and build process (now using lerna)

re #175

…actions; renamings and minors

This monorepo approach where the CLI commands have a dedicated package of actions and helpers, the
backend is organized in a more generic fashion will ease the burden of testing and maintainability.

BREAKING CHANGE: The folder structure and build process (now using lerna)

re #175
@0xjei 0xjei added the Refactoring ♻ Modify internal without changing external label Nov 17, 2022
@0xjei 0xjei added this to the [E1 - MS1] Testing <Bootstrap> milestone Nov 17, 2022
@0xjei 0xjei self-assigned this Nov 17, 2022
@0xjei 0xjei changed the title Monorepo WIP - Monorepo Nov 17, 2022
@baumstern baumstern linked an issue Nov 18, 2022 that may be closed by this pull request
6 tasks
@0xjei 0xjei changed the title WIP - Monorepo Monorepo Nov 23, 2022
@0xjei 0xjei marked this pull request as ready for review November 24, 2022 14:36
jest.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@baumstern

This comment was marked as resolved.

package.json Show resolved Hide resolved
@baumstern

This comment was marked as resolved.

@baumstern
Copy link
Contributor

In general, it looks like package structure is neat and easy to understand, also easy to extend. Great work!

@ctrlc03
Copy link

ctrlc03 commented Nov 29, 2022

In general, it looks like package structure is neat and easy to understand, also easy to extend. Great work!

+1 great stuff Jei

@0xjei
Copy link
Contributor Author

0xjei commented Nov 29, 2022

Not directly relevant this PR but think COPYRIGHT HOLDER part of LICENSE file should be organisation.

Yes, I think we should talk about licensing internally w/ the team. BTW, I will start switching from "myself" to EF or PSE (MACI/QFI) team whenever needed.

@0xjei
Copy link
Contributor Author

0xjei commented Nov 29, 2022

How about to move packages/actions directory to packages/phase2cli/src/actions? Exported functions of packages/actions are highly context dependent of phase2cli. (The directory structure like packages/actions/src/core/auth gives impression that it will provide complete functionality of authentication, since it has general naming)

My original idea was to disassemble the core of the CLI as much as possible from what pertains to the UI (e.g., console.log, theme). That way, it would be possible for anyone to write their own interface of the CLI and extend/improve the core easily.

Now, as you correctly stated, the actions package has a strong dependency on the CLI. So, def. your proposal makes more than sense because we could still work on simplify the testing process and write modular code. Also, the dependency between the two packages would be removed.

I think what I was initially trying to do (complete modularization) is more suitable in the future when the suite will become a more mature toolset.

What are your thoughts? And thanks for the great insight!

@ctrlc03
Copy link

ctrlc03 commented Nov 29, 2022

How about to move packages/actions directory to packages/phase2cli/src/actions? Exported functions of packages/actions are highly context dependent of phase2cli. (The directory structure like packages/actions/src/core/auth gives impression that it will provide complete functionality of authentication, since it has general naming)

My original idea was to disassemble the core of the CLI as much as possible from what pertains to the UI (e.g., console.log, theme). That way, it would be possible for anyone to write their own interface of the CLI and extend/improve the core easily.

Now, as you correctly stated, the actions package has a strong dependency on the CLI. So, def. your proposal makes more than sense because we could still work on simplify the testing process and write modular code. Also, the dependency between the two packages would be removed.

I think what I was initially trying to do (complete modularization) is more suitable in the future when the suite will become a more mature toolset.

What are your thoughts? And thanks for the great insight!

Personally I agree with Jei on this. Separating everything now will make it easier for anyone to build their own cli/ui by using the actions package.

@0xjei 0xjei marked this pull request as draft November 29, 2022 13:52
@0xjei
Copy link
Contributor Author

0xjei commented Nov 29, 2022

How about to move packages/actions directory to packages/phase2cli/src/actions? Exported functions of packages/actions are highly context dependent of phase2cli. (The directory structure like packages/actions/src/core/auth gives impression that it will provide complete functionality of authentication, since it has general naming)

My original idea was to disassemble the core of the CLI as much as possible from what pertains to the UI (e.g., console.log, theme). That way, it would be possible for anyone to write their own interface of the CLI and extend/improve the core easily.

Now, as you correctly stated, the actions package has a strong dependency on the CLI. So, def. your proposal makes more than sense because we could still work on simplify the testing process and write modular code. Also, the dependency between the two packages would be removed.

I think what I was initially trying to do (complete modularization) is more suitable in the future when the suite will become a more mature toolset.

What are your thoughts? And thanks for the great insight!

Whenever you have some time. FYI ^^^ @daodesigner

Copy link
Collaborator

@daodesigner daodesigner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nits but looks good overall! Great job 😊

jest.json Show resolved Hide resolved
jest.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/actions/src/index.ts Show resolved Hide resolved
packages/phase2cli/README.md Show resolved Hide resolved
packages/phase2cli/src/lib/auth.ts Show resolved Hide resolved
packages/phase2cli/src/lib/constants.ts Show resolved Hide resolved
packages/phase2cli/src/lib/firebase.ts Outdated Show resolved Hide resolved
packages/phase2cli/types/index.ts Show resolved Hide resolved
@baumstern
Copy link
Contributor

How about to move packages/actions directory to packages/phase2cli/src/actions? Exported functions of packages/actions are highly context dependent of phase2cli. (The directory structure like packages/actions/src/core/auth gives impression that it will provide complete functionality of authentication, since it has general naming)

My original idea was to disassemble the core of the CLI as much as possible from what pertains to the UI (e.g., console.log, theme). That way, it would be possible for anyone to write their own interface of the CLI and extend/improve the core easily.
Now, as you correctly stated, the actions package has a strong dependency on the CLI. So, def. your proposal makes more than sense because we could still work on simplify the testing process and write modular code. Also, the dependency between the two packages would be removed.
I think what I was initially trying to do (complete modularization) is more suitable in the future when the suite will become a more mature toolset.
What are your thoughts? And thanks for the great insight!

Personally I agree with Jei on this. Separating everything now will make it easier for anyone to build their own cli/ui by using the actions package.

It makes sense you two. I agree on a package separation would help to enforce decoupling code dependencies and reminds to write modular code :)

@0xjei
Copy link
Contributor Author

0xjei commented Dec 2, 2022

How about to move packages/actions directory to packages/phase2cli/src/actions? Exported functions of packages/actions are highly context dependent of phase2cli. (The directory structure like packages/actions/src/core/auth gives impression that it will provide complete functionality of authentication, since it has general naming)

My original idea was to disassemble the core of the CLI as much as possible from what pertains to the UI (e.g., console.log, theme). That way, it would be possible for anyone to write their own interface of the CLI and extend/improve the core easily.
Now, as you correctly stated, the actions package has a strong dependency on the CLI. So, def. your proposal makes more than sense because we could still work on simplify the testing process and write modular code. Also, the dependency between the two packages would be removed.
I think what I was initially trying to do (complete modularization) is more suitable in the future when the suite will become a more mature toolset.
What are your thoughts? And thanks for the great insight!

Whenever you have some time. FYI ^^^ @daodesigner

We can call on this too. The package structure will have three packages right now:

  • phase2cli: CLI interface
  • backend: Firebase Cloud Functions + config/scripts for managing the instance (+ S3 config)
  • actions: core actions for CL-based interfaces

@0xjei 0xjei marked this pull request as ready for review December 2, 2022 09:55
Bootstrap basic data generators and utilities for testing
Use re-export instead of import/export
@daodesigner
Copy link
Collaborator

Lgtm!

package.json Show resolved Hide resolved
@0xjei 0xjei dismissed daodesigner’s stale review December 6, 2022 17:36

Agreed on meeting call, let's merge!

@0xjei 0xjei removed the request for review from daodesigner December 6, 2022 17:36
@0xjei 0xjei merged commit 55428b4 into dev Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority 🔥 Refactoring ♻ Modify internal without changing external
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert to a monorepo
4 participants