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

Topics currently implicitly solved #5

Open
DavidBruant opened this issue Dec 27, 2019 · 8 comments
Open

Topics currently implicitly solved #5

DavidBruant opened this issue Dec 27, 2019 · 8 comments

Comments

@DavidBruant
Copy link
Contributor

If there is an intention to start using layer-cake i feel a couple of topics need to be made explicit (at least in terms of intention being documented in the open and likely a couple of automated tests)

getter/setters

Are these meant to be composed?
Currently they are by the use of Object.getOwnPropertyDescriptors, but there are no automated tests on this topic, so it's not clear whether it's intentional and meant to stay

self mutability during object construction

In the PR, the final object is passed to layerFun as is and is mutable
It's not clear to me how much this is expected or a concern from a security point of view

final object mutability

The object returned from makeClassCake and makeTraitCake is mutable (at least in the sense of property addition)
Again, it's not clear to me whether this is intentional
However, i'm not sure it's necessarily a choice this lib should be making. Maybe this choice should be left to the consumer

@erights
Copy link
Member

erights commented Dec 27, 2019

On getter/setter. Yes. The use of descriptors is on purpose, intended to support accessor properties by copying the descriptors, rather than calling the getter.

@erights
Copy link
Member

erights commented Dec 27, 2019

On self mutability: It is a concern. In light of your question, I went back and looked, and it is in fact a bit worse than I thought. The layerFuns get the finalObject in a non-frozen form and cannot be prevented from mucking it up. The intent is that non-interference policy be expressed by the combineFunction, but it really can't do much about that. Altogether, I think it is fine, but should probably worry about it more. Nevertheless, you should proceed.

@erights
Copy link
Member

erights commented Dec 27, 2019

On final object mutability: I originally wrote this without any non-test use of harden because it started as an expository experiment I didn't want to make too specific for our uses. Now that it is specific to us, I think there a several places we should put in harden calls:

  • Around the record returned by each layer. IOW return self => harden({.
  • The return of the fully composed finalObject at the end of makeCake.

@erights
Copy link
Member

erights commented Dec 27, 2019

These will bring this code closer to being valid Jessie. However, neither before or after this PR can be valid Jessie by current Jessie rules. For the before, because Jessie has no generators. For the after, because finalObject is being passed around in a non-frozen state. Also, the whole descriptor notion is outside Jessie, which applies both before and after.

Although we should still keep this code as close to being in Jessie as we can, I think the appropriate level of ambition is that it be in SES but made available to Jessie, in the same way we expect to keep Proxy and Reflect out of Jessie, but to use them in SES to provide to Jessie a Jessie-friendly membrane abstraction. The key thing is that the layer code, and the call to makeClassCake and makeTraitCake for composing these layers, can all be in Jessie. I do not include makeCakeMaker because interesting confineLayers are likely not in Jessie.

@DavidBruant
Copy link
Contributor Author

The layerFuns get the finalObject in a non-frozen form and cannot be prevented from mucking it up. The intent is that non-interference policy be expressed by the combineFunction, but it really can't do much about that.

I see a possible solution: finalObject could be split into a target and a proxy. The lib has access to the target and can mutate it directly to combine the layers. The lib passes to each layer function only the proxy. If a layer tries to mutate the proxy directly (as opposed to via method calls), the proxy throws

@DavidBruant
Copy link
Contributor Author

On getter/setter. Yes. The use of descriptors is on purpose, intended to support accessor properties by copying the descriptors, rather than calling the getter.

Sounds good, keeping the issue open to remember to add automated tests for this

@DavidBruant
Copy link
Contributor Author

@erights I see another concer which is currently not explciitely covered in test cases: In case of naming conflicts, class-based cake favors the top-most layer. However, the case for trait-base cake is less clear to me. When there is a conflict, should makeTraitCake throw?
It currently does via Object.defineProperties which throws when trying to change the value of a non-configurable property

@erights
Copy link
Member

erights commented Jan 8, 2020

The intent of this simple makeTraitCake is indeed that it should throw; thereby enforcing a policy that traits are completely disjoint. That is the right policy for the use we're currently making of trait-like composition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants