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

[WIP] Upgraded to Preact 10.3.4 #5639

Open
wants to merge 9 commits into
base: master
from

Conversation

@nickytonline
Copy link
Member

nickytonline commented Jan 22, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Preact has been upgraded to the latest version, Preact X. This gives us hooks, componentDidCatch, fragments etc. See the official release for the whole rundown. Here is the official migration guide for Preact 8.x to X if you're curious why some changes have been made.

I've done a smoke test of the site on my local. Onboarding worked fine, search is good and the site seems to behaving normally.

Storybook appears to be broken still. This is related to work that needs to be done in #3490 and #2034. I will address that in a separate PR.

Related Tickets & Documents

#4243

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

Some cool X gif

@nickytonline nickytonline requested a review from maestromac as a code owner Jan 22, 2020
@nickytonline nickytonline requested a review from Jan 22, 2020
@pr-triage pr-triage bot added the PR: unreviewed label Jan 22, 2020
@@ -1,4 +1,3 @@
import 'preact/devtools';

This comment has been minimized.

Copy link
@nickytonline

nickytonline Jan 22, 2020

Author Member

I've removed preact/devtools as it no longer exists. preact/debug is what should be used, but it should only be for development purposes. I will address this in another PR.

See Debugging Preact Apps

This comment has been minimized.

Copy link
@maestromac

maestromac Jan 22, 2020

Member

Thanks for catching this!

@@ -1,9 +1,9 @@
import { h, render } from 'preact';
import { Search } from '../src/components/Search';
import 'focus-visible'
import 'focus-visible';

This comment has been minimized.

Copy link
@nickytonline

nickytonline Jan 22, 2020

Author Member

A little prettier action happening in a few files.


document.addEventListener('DOMContentLoaded', () => {
const root = document.getElementById('nav-search-form-root');

render(<Search />, root, root.firstElementChild);
render(<Search />, root);

This comment has been minimized.

Copy link
@nickytonline

nickytonline Jan 22, 2020

Author Member

This is part of the upgrade. VDOM diffing works the way it does in most other libraries, e.g. React.

@nickytonline

This comment has been minimized.

Copy link
Member Author

nickytonline commented Jan 22, 2020

The codeclimate issues appear to be complaining about code duplication for PropTypes but it's not an issue. The prop types are different in each case.

The other issue is related to jest issues as test scores have gone from A -> F 😬

@nickytonline

This comment has been minimized.

Copy link
Member Author

nickytonline commented Jan 22, 2020

I need to sort out the tests in the frontend as a lot of the snapshots changed, but the structure is different and information appears to be missing. Maybe Preact X is not compatible with some of the tools we were using for snapshot testing with Preact. Either way, gotta get it sorted. 😉

image

@nickytonline nickytonline changed the title Upgrade to Preact X [WIP] Upgrade to Preact X Jan 22, 2020
@pr-triage pr-triage bot removed the PR: unreviewed label Jan 22, 2020
@nickytonline

This comment has been minimized.

Copy link
Member Author

nickytonline commented Jan 22, 2020

Tiny update. It looks like there are issues with preact-render-spy and Preact X. See mzgoddard/preact-render-spy#92.

@mstruve mstruve removed the request for review from Jan 22, 2020
@nickytonline nickytonline force-pushed the nickytonline/upgrade-to-preact-x branch from 2a6deaf to 516f504 Jan 22, 2020
@maestromac maestromac removed their request for review Jan 22, 2020
@nickytonline

This comment has been minimized.

Copy link
Member Author

nickytonline commented Jan 23, 2020

I spoke briefly with Jason Miller from Preact, and he is going to look into getting preact-render-spy etc. fixed up. So for the time being, I will put this PR on hold.

@nickytonline nickytonline changed the title [WIP] Upgrade to Preact X [WIP] Upgraded to Preact 10.3.4 Mar 24, 2020
@nickytonline nickytonline force-pushed the nickytonline/upgrade-to-preact-x branch from 822921e to a28fe97 Mar 24, 2020
@nickytonline nickytonline requested review from thepracticaldev/oss as code owners Mar 24, 2020
@nickytonline nickytonline requested review from rhymes and removed request for thepracticaldev/oss Mar 24, 2020
@rhymes rhymes removed their request for review Mar 24, 2020
@joshpuetz

This comment has been minimized.

Copy link
Member

joshpuetz commented Mar 27, 2020

@nickytonline Seems like https://github.com/mzgoddard/preact-render-spy might be abandoned: there's a Preact X fork up at https://github.com/shelacek/preact-render-spy. What do we use preact-render-spy for anyway? Is using a pure enzyme approach like https://github.com/preactjs/enzyme-adapter-preact-pure a replacement?

@nickytonline

This comment has been minimized.

Copy link
Member Author

nickytonline commented Mar 27, 2020

@nickytonline Seems like mzgoddard/preact-render-spy might be abandoned: there's a Preact X fork up at shelacek/preact-render-spy. What do we use preact-render-spy for anyway? Is using a pure enzyme approach like preactjs/enzyme-adapter-preact-pure a replacement?

@joshpuetz, I tried the fork of preact-render-spy and it does not work. We use preact-render-spy for snapshot testing. It does appear to be abandoned although Jason Miller talked about fixing it up. See this Tweet thread between Jason and I.

Also, I'm looking to move away from snapshot testing in favour of preact-testing-library. Even enzyme is not considered to be the way to test React components now (so Preact as well). As I mentioned on Slack, react-testing-library is the recommended tool by the React team, so I would be inclined to move towards preact-testing-library.

The way I see it panning out is migrating to another testing tool and once all tests are migrated, we can upgrade.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.