-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Fix] render
: handle Fiber strings and numbers
#2221
Conversation
b530e0c
to
4ae867f
Compare
Rebased and fixed linter errors |
4ae867f
to
701ae97
Compare
701ae97
to
674e233
Compare
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.
Thanks!
674e233
to
922a6e3
Compare
922a6e3
to
cf240c8
Compare
I've rebased again, including my latest changes as well as your utils spec. |
That was quick, thank you very much! I didn't see your approval while I was adding the utils spec. I added them because the coverage went down. With your latest push everything is as it should be 👍 |
This build will fail: https://travis-ci.org/airbnb/enzyme/jobs/585805429
I will push a fix shortly |
cheerio does need to be a dep of the test suite, but that doesn't fix it. I'll wait for your push. |
cf240c8
to
8016139
Compare
I used the public property of the https://github.com/cheeriojs/cheerio/blob/1.0.0-rc.3/lib/cheerio.js#L101 Cheerio.prototype.cheerio = '[cheerio object]'; It is only available on Cheerio instances: $ node
> const cheerio = require('cheerio');
undefined
> cheerio.version
'1.0.0-rc.3'
> cheerio.cheerio
undefined
> cheerio('<div>foo</div>').cheerio
'[cheerio object]'
> cheerio.load('')().cheerio
'[cheerio object]'
> |
eesh, i mean, that clearly works but that doesn't seem robust enough to be a valuable test. why was instanceOf(cheerio) failing? it works in the repl. |
The cheerio : "0.0.1" It was later changed to hold the special value - cheerio : '0.4.2',
+ cheerio : '[cheerio object]', As it is also used in However, I didn't look into why the > const cheerio = require('cheerio');
undefined
> cheerio.version
'1.0.0-rc.3'
> cheerio.root() instanceof cheerio
true
> cheerio.load('<div>foo</div>', null, false).root() instanceof cheerio
true
> cheerio.load('')('<div>foo</div>') instanceof cheerio
true
> I will investigate and submit a new version when I found the issue |
Thanks! The issue isn't whether the property works in every version of cheerio - this is knowable - but whether any object can mistakenly pass the test by having an arbitrary string property. In tests of our own code, this really isn't a big concern, just a philosophical one :-) really, it's that i'd prefer to at least understand why no better option is available before accepting a non-ideal one. |
What I found out so far is that there are two different identities for
Which means the versions are identical but
This seems to be more of a build setup issue, did you run into this problem before? |
Also reproducible in enzyme/packages/enzyme-test-suite$ node
> const cheerio = require('cheerio');
undefined
> const loadCheerioRoot = require('enzyme/build/Utils').loadCheerioRoot;
undefined
> loadCheerioRoot().constructor.version === cheerio.version
true
> String(loadCheerioRoot().constructor) === String(cheerio)
true
> loadCheerioRoot().constructor === cheerio
false
> |
gotcha, now that makes sense :-) i'd expect lerna to hoist them up so they're the same version, but either way this is something I can solve. Will fix shortly. |
8305b85
to
4b3cc31
Compare
k, i ended up going with your test suggestion ¯\_(ツ)_/¯ |
bb08dfb
to
9aa653f
Compare
- remove redundant jobs
- `enzyme-adapter-utils` `assertDomAvailable` & `elementToTree` - `EnzymeAdapter` `matchesElementType`
9768184
to
1ac0daf
Compare
cd6e165
to
e27f3bc
Compare
…Wrapper; add tests Technically this could be considered a breaking change since it’s a `class` and these are public methods on it. *However*, since nobody should be depending on this package directly, and since enzyme itself doesn’t appear to use it, I’m going to consider it a patch. In the event that it is a breaking change for someone, I’ll add back the methods, ideally as no-ops.
- [enzyme] [fix] `render`: handle Fiber strings and numbers - [enzyme] [new] `Utils`: add `loadCheerioRoot` Fixes enzymejs#2098
e27f3bc
to
d9cc09e
Compare
@ljharb Thanks for wrapping this up! |
New Stuff - `render`: handle Fiber strings and numbers (#2221) Fixes - `shallow`: Share child context logic between `shallow` and `dive` (#2296) - `mount`: `children`: include text nodes ($2269) - `mount`: `invoke`: use adapter’s `wrapInvoke` if present (#2158) Docs - `mount`/`shallow`: `closest`/`parent`: Add missing arguments description (#2264) - `mount`/`shallow`: fix pluralization of “exist” (#2262) - `shallow`/`mount`: `simulate`: added functional component example to simulate doc (#2248) - `mount`: `debug`: add missing verbose option flag (#2184) - `mount`/`shallow`: `update`: fix semantics description (#2194) - add missing backticks to linked method names (#2170) - `invoke`: Add missing backticks to end of codeblock (#2160) - `invoke`: Fix typo (#2167) - Explicit React CSS selector syntax description (#2178) Meta Stuff - [meta] add `funding` field - [meta] Update airbnb.io URLs to use https (#2222) - [deps] update `is-boolean-object`, `is-callable`, `is-number-object`, `is-string`, `enzyme-shallow-equal`, `array.prototype.flat`, `function.prototype.name`, `html-element-map`, `is-r egex`, `object-inspect`, `object-is`, `object.entries`, `object.vales`, `raf`, `string.prototype.trim` - [dev deps] update `eslint`, `eslint-plugin-import`, `eslint-plugin-markdown`, `eslint-plugin-react`, `safe-publish-latest`, `eslint-config-airbnb`, `rimraf`, `safe-publish-latest`, `k arma-firefox-launcher`, `babel-preset-airbnb`, `glob-gitignore`, `semver`, `eslint-plugin-jsx-a11y`
loadCheerioRoot
utilrender
,mount
andshallow
test/staticRender-spec.jsx