Make private properties more private and harder to use #1083
Conversation
lelandrichardson
force-pushed
the
lmr--move-to-private-props
branch
from
fce43fe
to
bf3aebd
Compare
packages/enzyme/src/ReactWrapper.jsx
Outdated
| get() { | ||
| throw new Error(` | ||
| Attempted to access ShallowWrapper::${prop}, which was previously a private property on | ||
| Enzyme ShallowWrapper instances, but is no longer and should not be relied upon. |
There was a problem hiding this comment.
Need to change ShallowWrapper to ReactWrapper.
lelandrichardson
force-pushed
the
lmr--move-to-private-props
branch
from
bf3aebd
to
c67077b
Compare
lelandrichardson
force-pushed
the
lmr--move-to-private-props
branch
from
c67077b
to
5614f92
Compare
|
It'd be cool if we could go a bit slower on merging these things; it's Sunday and cell reception is limited on my drive home from out of town. |
|
@ljharb i'll slow down... i'm trying to take advantage of time when i have it though, and some of these PRs are hard to do in parallel. |
|
PRs don't have to block proceeding on multiple local branches, and rebasing/PRing as things are merged :-) |
| @@ -4300,14 +4300,14 @@ describe('shallow', () => { | |||
| it('works with a name', () => { | |||
| const wrapper = shallow(<div />); | |||
| wrapper.single('foo', (node) => { | |||
| expect(node).to.equal(wrapper.node); | |||
| expect(node).to.equal(wrapper[sym('__node__')]); | |||
There was a problem hiding this comment.
Can these underscores be added by the sym function, instead of hardcoded in the text?
| @@ -1060,4 +1071,25 @@ if (ITERATOR_SYMBOL) { | |||
| }); | |||
| } | |||
|
|
|||
| function privateWarning(prop, extraMessage) { | |||
There was a problem hiding this comment.
this function seems like it could live in a separate file and be shared, and take the proto as an argument?
| @@ -362,3 +362,15 @@ export function displayNameOfNode(node) { | |||
|
|
|||
| return type.displayName || (typeof type === 'function' ? functionName(type) : type.name || type); | |||
| } | |||
|
|
|||
| export function sym(s) { | |||
| return typeof Symbol === 'function' ? Symbol.for(`enzyme.${s}`) : s; | |||
There was a problem hiding this comment.
the presence of Symbol doesn't ensure Symbol.for is present - this should maybe be a fallback from Symbol.for to a memoized Symbol to `___${s}___`?
There was a problem hiding this comment.
can you explain what you're proposing a little bit more? i'm not sure I understand
There was a problem hiding this comment.
const syms = {};
function sym(s) {
const k = `enzyme.___${s}___`;
if (typeof Symbol === 'function') {
if (typeof Symbol.for === 'function') return Symbol.for(k);
const sym = Symbol(k);
syms[s] = sym;
return sym;
}
return k;
}| } | ||
|
|
||
| export function privateSet(obj, prop, value) { | ||
| Object.defineProperty(obj, prop, { |
There was a problem hiding this comment.
any reason not to use a privateGet as well, so it could later be refactored to use WeakMaps?
There was a problem hiding this comment.
no real reason, no. It would add some syntactic overhead relative to obj[prop], but i'm not opposed to it if we want to. I figured we'd just keep this setup until private properties are at whatever stage makes you comfortable with us using them :)
There was a problem hiding this comment.
that's exactly why i want them to use WeakMap, because that's how private properties will be transpiled :-)
to: @ljharb @aweary
This is in preparation for Enzyme v3.0. We are moving properties that are considered private and implementation details to use symbols and harder to rely on property names in an effort to make future migrations easier.