-
Notifications
You must be signed in to change notification settings - Fork 11
Fix deepFreeze #657
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
base: master
Are you sure you want to change the base?
Fix deepFreeze #657
Conversation
src/sbvr-api/permissions.ts
Outdated
| obj.hasOwnProperty(prop) && | ||
| obj[prop] !== null && | ||
| !['object', 'function'].includes(typeof obj[prop]) | ||
| // `synonyms` is apparently changed later on and cannot be deepFroozen |
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.
Where is synonyms modified?
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.
@Page-
In the get Proxy here:
pinejs/src/sbvr-api/permissions.ts
Line 983 in 6675183
| synonyms[`${synonym}$${alias}`] = `${canonicalForm}$${alias}`; |
src/sbvr-api/permissions.ts
Outdated
| }); | ||
| }; | ||
|
|
||
| const deepFreezeExceptDefinition = (obj: AnyObject) => { |
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.
ExceptDefinition isn't accurate to what this is attempting to do
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.
Yep corrected.
|
|
||
| const table = tables[`${resourceName}$bypass`]; | ||
|
|
||
| const permissionsTable = (tables[permissionResourceName] = { |
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.
If you don't assign to tables[permissionResourceName] then all these changes to permissionsTable are essentially noops?
6675183 to
8efc9f3
Compare
Actually deep freezing the `constrainedAbstractSqlModel` Change-type: patch Signed-off-by: Harald Fischer <harald@balena.io> Signed-off-by: fisehara <harald@balena.io>
8efc9f3 to
d285613
Compare
| // `synonyms` is changed later on in the constrainedAbstractSqlModel.symbols get proxy | ||
| // `tables` is changed later on in the constrainedAbstractSqlModel.tables get proxy | ||
| !['synonyms', 'tables'].includes(prop) && |
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.
Does the _.cloneDeep in
pinejs/src/sbvr-api/permissions.ts
Line 963 in 6675183
| const constrainedAbstractSqlModel = _.cloneDeep(abstractSqlModel); |
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.
@Page- from my understanding the constrainedAbstractSqlModel should be deep frozen after the deepClone was made.
The freezing part applies to the constrainedAbstractSqlModel and at runtime the Proxies manipulate the dee frozen Object by writing parts to it through the proxies.
So I don't see that the _.cloneDeep is part of the freeze, please correct me here!
Actually deep freezing the
constrainedAbstractSqlModelChange-type: patch