- Notifications
You must be signed in to change notification settings - Fork772
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat(facade): add unit hooks#3158
base:dev
Are you sure you want to change the base?
Conversation
| for (const callback of this.beforeUnitAddCallbacks) { | ||
| if (callback(unit) === false) { | ||
| // if not use throw error, the createUnit returns null | ||
| throw new Error('[UniverInstanceService]: cannot add unit with user callback.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@wzhudev Check if this has been handled properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know. It is your responsibility to check if it is handled properly.
View Deployment
|
| }); | ||
| // TODO: not calling dispose() method | ||
| // unit.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@wzhudev The unit.dispose() function is not associated; in the future, a better and more unified mechanism is needed. Consider movingdispose toUnitModel or an intermediate layer. Usedispose for the destroy function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Already clearified this with hexf00 offline.
8a0b16c to0e106b7CompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## dev #3158 +/- ##==========================================+ Coverage 27.63% 27.65% +0.01%========================================== Files 1984 1985 +1 Lines 105556 105605 +49 Branches 22792 22795 +3 ==========================================+ Hits 29172 29200 +28- Misses 76384 76405 +21 ☔ View full report in Codecov by Sentry. |
0e106b7 to7423d7cCompare| @@ -47,6 +47,11 @@ export interface ICreateUnitOptions { | |||
| * It also manages the focused univer instance. | |||
| */ | |||
| export interface IUniverInstanceService { | |||
| /** A set of callbacks that will be called before a new unit is added. */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should never expose internal properties as mutable. Instead, provide methods to do that.
| // TODO: not calling dispose() method | ||
| // unit.dispose(); | ||
| const _univerInstanceService = get(IUniverInstanceService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not necessary to add an underscore before a variable. The underbar has a special usage for "not used variables".
| }); | ||
| // TODO: not calling dispose() method | ||
| // unit.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Already clearified this with hexf00 offline.
Hooks naming will be standardized to the format: prefix + verb in present tense + noun.
Pull Request Checklist