Migrate to a monorepo#297
Conversation
piotr-roslaniec
requested review from
KPrasch,
cygnusv,
derekpierre,
jMyles,
manumonti and
theref
| import * as predefined from './predefined'; | ||
|
|
||
| export { predefined, base }; | ||
| // TODO: Or do we want to export everything from the base and predefined modules? |
There was a problem hiding this comment.
Could you elaborate a bit on this? WDYM with structured exports vs export everything from the base?
There was a problem hiding this comment.
Here we're exporting everything in a shared namespace:
import * as base from './base';
import * as predefined from './predefined';
So everything ends up being available in the project root module:
import {Condition, ERC721Ownership} from "@nucypher/shared";
Previously, we used named module exports:
export { predefined, base };
And used them like so:
import {based, predefined} from "@nucypher/shared";
// use base.Condition, predefined.ERC721Ownership
We could use both. Not sure yet - I need to sort out contents of pre first.
manumonti
left a comment
manumonti
left a comment
There was a problem hiding this comment.
This is great. I've left some minor suggestions
Codecov Report
@@ Coverage Diff @@
## monorepo #297 +/- ##
=============================================
- Coverage 81.38% 65.23% -16.15%
=============================================
Files 37 48 +11
Lines 1069 5218 +4149
Branches 112 35 -77
=============================================
+ Hits 870 3404 +2534
- Misses 191 1808 +1617
+ Partials 8 6 -2
|
| @@ -0,0 +1,9 @@ | |||
| root = true | |||
There was a problem hiding this comment.
Is this file supposed to be in the repo or maybe should it be ignored?
There was a problem hiding this comment.
It's supposed to be in repo: https://editorconfig.org/
| @@ -1,365 +0,0 @@ | |||
| # Changelog | |||
There was a problem hiding this comment.
What happens with this changelog?
There was a problem hiding this comment.
I'm unsure how to transition from the old changelog to the new one. So far I figured that:
nucypher-tspackage will be discontinued and superseded bytaco,pre, etc. packages- We can make appropriate changes to changelog upon new package release
- We could put the old changelog at the bottom of the old changelog
- Or we could put it into
CHANGELOG.old.md - Irregardless of the two above, we could explain this transition in the
README.md
| @@ -0,0 +1,64 @@ | |||
| import {Alice, Bob, getPorterUri, SecretKey, toBytes} from '@nucypher/shared'; | |||
There was a problem hiding this comment.
This is a PRE example...I found confusing to find this here. It should be moved somewhere else and/or clearly marked in the filename/path.
There was a problem hiding this comment.
I addressed these changes in another PR, #301
| return <div>Loading...</div>; | ||
| } | ||
|
|
||
| const { Alice, Bob, EnactedPolicy, getPorterUri, SecretKey, toHexString } = nucypher; |
| export * from './bob'; | ||
| export * from './cbd-recipient'; | ||
| export * from './enrico'; | ||
| export * from './pre-recipient'; |
There was a problem hiding this comment.
What's the difference between bob and pre-recipient?
There was a problem hiding this comment.
Bob is a vanilla PRE character, PreDecrypter from ./pre-recipient is a tdec-adapted Bob.
There was a problem hiding this comment.
I see, well, we should deprecate PreDecryper as soon as possible, but I guess that's something that can wait after this PR.
There was a problem hiding this comment.
Ok, I will deprecate the reminder of PRE-tDec - #303
| @@ -0,0 +1,2145 @@ | |||
| /* Autogenerated file. Do not edit manually. */ | |||
There was a problem hiding this comment.
When and how is this file autogenerated?
There was a problem hiding this comment.
This file is generated by typechain by postinstall and build npm scripts.
KPrasch
left a comment
KPrasch
left a comment
There was a problem hiding this comment.
This is a huge organizational effort!
While I cannot say that this is the final form of the repository and packaging manifests, as a whole you present a massively improved DX for both contributors and implementers and a division of logic that resolves technical debt. I need more experience using the demos and building TACo dapps before I can make any further API suggestions but I broadly agree with the internal tool choices you made here. To unblock further efforts this PR must be merged along with the follow-ups.
Your meticulous effort and focus on maintainability haven’t gone unnoticed. It’s a solid step forward in both code quality and efficiency. On this note, I’m greenlighting the merge. Thanks @piotr-roslaniec 👏🏻
5 participants
To Do
Consider moving to separate issues/PRs
changesetsfor package release management. Need to redo theCHANGELOG.md, too. - Configure automated releases withchangesets#304nucypher-tsdemos (like https://github.com/nucypher/nucypher-ts-demo-tdec) as examples in./demos- Refactor packages APIs #301webpack-5andwebpack-bundlerexamples are building, but are not actually working correctly. Fix them and consider adding examples for other bundlers. Consider adding e2e tests for integration examples (to be run on CI). - Improve DX when working with WASM dependencies #299prepackage, similar totacopackages. - Delayed, some parts addressed on Refactor CBD API #231preandtacomodules. - Refactor packages APIs #301Type of PR:
Required reviews:
What this does:
shared,pre, andtacopackagespnpmas a package managerjestwithvitest./packages/test-utilspackage to deduplicate tests across packagesNotes for reviewers:
06f95adis the largest because it moves a lot of code. You may want to view it separately. Other commits are more self-contained.taco,pre, andsharedtest-utilspackage is not meant to be published.preandtacowill be published. Consider publishing asharedpackage. Package naming TBD../examplesare using@nucypher/shared. Ideally, they would be usingpreandtacowhere appropriate. We need to figure out the structure ofprepackage before migrating them, and we need to addtacousage examples.