eps1lon commented 2 years ago

Compared to https://github.com/enzymejs/enzyme/pull/2534/:

less React 17 branching in tests (goal is 0 branching but branching for error stacks is probably fine)
some legacy code removed

TODO: Open to discussing the remaining items @ljharb

[ ] callstack/componentstack Would need to discuss if we want to normalize them across React versions. For the initial release I would lean towards being ok with different stacks between 16 and 17 since that's also what developers experience.
[x] hide internal Suspense fibers
[x] Green CI
[x] code coverage hygiene

Closes https://github.com/enzymejs/enzyme/issues/2429 Closes https://github.com/enzymejs/enzyme/pull/2534 Closes https://github.com/enzymejs/enzyme/pull/2430

codecov[bot] commented 2 years ago

Codecov Report

Base: 96.31% // Head: 95.99% // Decreases project coverage by -0.32% :warning:

Coverage data is based on head (8649c0c) compared to base (538b0d8). Patch coverage: 93.32% of modified lines in pull request are covered.

:exclamation: Current head 8649c0c differs from pull request most recent head 8712dca. Consider uploading reports for the commit 8712dca to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #2564 +/- ## ========================================== - Coverage 96.31% 95.99% -0.33% ========================================== Files 49 53 +4 Lines 4207 4716 +509 Branches 1130 1279 +149 ========================================== + Hits 4052 4527 +475 - Misses 155 189 +34 ``` | [Impacted Files](https://codecov.io/gh/enzymejs/enzyme/pull/2564?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs) | Coverage Δ | | |---|---|---| | [packages/enzyme-adapter-utils/src/Utils.js](https://codecov.io/gh/enzymejs/enzyme/pull/2564/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs#diff-cGFja2FnZXMvZW56eW1lLWFkYXB0ZXItdXRpbHMvc3JjL1V0aWxzLmpz) | `96.26% <ø> (ø)` | | | [...pter-react-17/src/findCurrentFiberUsingSlowPath.js](https://codecov.io/gh/enzymejs/enzyme/pull/2564/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs#diff-cGFja2FnZXMvZW56eW1lLWFkYXB0ZXItcmVhY3QtMTcvc3JjL2ZpbmRDdXJyZW50RmliZXJVc2luZ1Nsb3dQYXRoLmpz) | `68.42% <68.42%> (ø)` | | | [...zyme-adapter-react-17/src/ReactSeventeenAdapter.js](https://codecov.io/gh/enzymejs/enzyme/pull/2564/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs#diff-cGFja2FnZXMvZW56eW1lLWFkYXB0ZXItcmVhY3QtMTcvc3JjL1JlYWN0U2V2ZW50ZWVuQWRhcHRlci5qcw==) | `96.07% <96.07%> (ø)` | | | [...ges/enzyme-adapter-react-17/src/detectFiberTags.js](https://codecov.io/gh/enzymejs/enzyme/pull/2564/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs#diff-cGFja2FnZXMvZW56eW1lLWFkYXB0ZXItcmVhY3QtMTcvc3JjL2RldGVjdEZpYmVyVGFncy5qcw==) | `100.00% <100.00%> (ø)` | | | [packages/enzyme-adapter-react-17/src/index.js](https://codecov.io/gh/enzymejs/enzyme/pull/2564/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs#diff-cGFja2FnZXMvZW56eW1lLWFkYXB0ZXItcmVhY3QtMTcvc3JjL2luZGV4Lmpz) | `100.00% <100.00%> (ø)` | | | [...pter-react-helper/src/getAdapterForReactVersion.js](https://codecov.io/gh/enzymejs/enzyme/pull/2564/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs#diff-cGFja2FnZXMvZW56eW1lLWFkYXB0ZXItcmVhY3QtaGVscGVyL3NyYy9nZXRBZGFwdGVyRm9yUmVhY3RWZXJzaW9uLmpz) | `100.00% <100.00%> (ø)` | | | [packages/enzyme/src/ShallowWrapper.js](https://codecov.io/gh/enzymejs/enzyme/pull/2564/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs#diff-cGFja2FnZXMvZW56eW1lL3NyYy9TaGFsbG93V3JhcHBlci5qcw==) | `99.13% <100.00%> (+0.01%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=enzymejs)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

ljharb commented 2 years ago

@eps1lon thanks! (but i wish you'd posted a branch link instead of opening a new PR - i now have three PRs to juggle (this, #2534, and #2430), which makes landing this update even harder)

ljharb commented 2 years ago

Re your discussion point, as long as component stacks are correct within a React major/minor, there's no reason they need to stay the same as those change.

eps1lon commented 2 years ago

@eps1lon thanks! (but i wish you'd posted a branch link instead of opening a new PR - i now have three PRs to juggle (this, #2534, and #2430), which makes landing this update even harder)

What do you need to juggle and how would a branch help? The existing 2 are replaced by this one. If the forked component stack tests are fine, then this PR is ready to land

ljharb commented 2 years ago

Every pull request has a permanent, eternal, undeleteable ref created on Github (that's not fetched by git by default, but can be opted into), so all three PRs must point to the same commit before landing any of them, or else any outliers will remain as orphaned refs for eternity.

eps1lon commented 2 years ago

Why are the other two PRs relevant here? They've been abandoned. The PR description clearly states that thus PR replaces them. How would I get CI to work without a PR?

Is there any change substance to discuss or do we want to spend more time on unrelated details? If I understood you correctly you have very limited time. But even if someone gives you a green PR that's not ok because?

ljharb commented 2 years ago

@eps1lon the PR refs are part of the github repo, and no PRs are truly abandoned when a maintainer can update them directly. Either way, repo management concerns are never irrelevant/unrelated. On all my projects, I strenuously avoid having any abandoned PRs, and I repurpose every PR to ensure that all PR refs point to something useful.

This PR isn't quite green - coverage is failing - but it's great that tests are passing. When I have time to review it, I will certainly do so, and I appreciate the contribution.

eps1lon commented 2 years ago

Are createClass components no longer supported in React 17?

I'm not aware of any changes to createClass. How would I tell the tests to test for createClass support? I was under the impression that would be automatic just like with the other adapter tests

ljharb commented 2 years ago

I think I diffed the 16 to 17 adapter files, and saw some code related to createClass removed.

In theory yes, the tests should be automatic, so if they're passing then maybe it's fine.

SwatiJadhav46 commented 2 years ago

@eps1lon Thanks for integrating this adapter. We are eagerly waiting for this to release, as it's a blocker for us in our project.

eps1lon commented 2 years ago

This PR isn't quite green - coverage is failing - but it's great that tests are passing.

@ljharb Is this something you want me to address? As far as I can tell this adapter has not less coverage than the 16 adapter. But since 16 adapter has less coverage than the average it's somewhat expected that coverage decreases. It looks to me that coverage target isn't really a target and more a check for changes (96.31% looks completely arbitrary to me).

ljharb commented 2 years ago

@eps1lon nah definitely not a requirement - just something that'd be nice to improve, if possible, until i have the time to get to the PR. Coverage targets are just minimums; the ideal is always 100% :-)

Prior99 commented 1 year ago

@ljharb We're in dire need to have support for react 17 and 18 in enzyme. Is there anything we could help with to speed things up?

mfv-brian commented 10 months ago

Hello, how is the current progress ?

somenickname commented 9 months ago

https://dev.to/wojtekmaj/enzyme-is-dead-now-what-ekl

aks- commented 5 months ago

Hi! I worked on trying to get adapter 17 to completion and out. Can you please take a look @ljharb? Here is the link

enzymejs / enzyme

add enzyme-adapter-react-17 #2564

Codecov Report