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

[NO-TICKET] Upgrade day picker #2920

Closed
wants to merge 2 commits into from

Conversation

dmethvin-gov
Copy link
Contributor

Summary

This is the current state of my attempt to upgrade react-day-picker. There is some problem in Storybook where it crashes, see below. I'm making a PR to see how it does on the rest of the tests.

In trying to revive Storybook I upgraded the version, the one currently being used is an alpha.

How to test

  1. Pull down the branch and update dependencies.
  2. Run tests
  3. yarn storybook and go to the SingleInputDateField, with Picker. Click the button.
Screenshot 2024-02-06 at 9 58 31 AM

Checklist

  • Prefixed the PR title with the Jira ticket number as [WNMGDS-####] Title or [NO-TICKET] if this is unticketed work.
  • Selected appropriate Type (only one) label for this PR, if it is a breaking change, label should only be Type: Breaking
  • Selected appropriate Impacts, multiple can be selected.
  • Selected appropriate release milestone

If this is a change to design:

  • If visual regression image references have been changed, design MUST be assigned to review. In this instance, designer approval is a requirement before the PR can be merged.

If this is a change to code:

  • Created or updated unit tests to cover any new or modified code
  • If necessary, updated unit-test snapshots (yarn test:unit:update) and browser-test snapshots (yarn test:browser:update)

If this is a change to documentation:

  • Checked for spelling and grammatical errors

@pwolfert
Copy link
Collaborator

pwolfert commented Feb 9, 2024

@dmethvin-gov, while it could indeed be a problem with Storybook, have you tried the upgraded component outside the context of Storybook? Like running it in one of the projects in the examples folder? Then we could rule out whether it's a storybook-specific issue. If it is a Storybook-specific issue, we could make a separate PR to upgrade Storybook and then move forward from there.

@dmethvin-gov
Copy link
Contributor Author

@pwolfert This PR upgrades Storybook, so doing that didn't help. The yarn storybook:react run of Storybook seems to work fine. It's only the default Preact run that fails, and it is blowing up in the new jsx transform trying to read a React internal variable that's undefined. I haven't tried upgrading the Preact dependencies yet but I can try; that might help.

As for examples, the create-react-app-typescript, preact-app, and preact-react-app examples work. The react-app example throws this error on yarn install:
Screenshot 2024-02-12 at 12 24 14 PM
and this error on yarn build:
Screenshot 2024-02-12 at 12 26 50 PM
Do you see something similar on your local setup?

@pwolfert
Copy link
Collaborator

I've at least been able to narrow down the issue a little bit in my investigations so far. Looks like it works in React and not Preact.

@pwolfert
Copy link
Collaborator

Narrowed it down to something that happened in that library between 8.8.2 and 8.9.0. That's when it stops working in Storybook with Preact.

@dmethvin-gov
Copy link
Contributor Author

It seems like the most likely commit to have caused the problem is gpbl/react-day-picker@f3327c9 because most everything else was dependency updates or minor changes. (diff of the two versions)

That would also make some sense because it's changing the way imports are done, along with several other settings changes. (tsconfig reference)

There's a preact issue that sounds like it might be similar, but that one in particular is about Vite.

@pwolfert
Copy link
Collaborator

I was thinking the same thing, that it was the removal of the React imports and changing of the JSX transform implementation, but I hadn't gathered enough evidence or figured out a way forward. Even though we still support React 16, I tried changing our JSX transform settings in the repo yesterday, but I still wasn't able to get it to work right. I see in the issue you linked to that someone had a Vite config setting for Storybook 7.6 that seemed to work. We could go down that road of discovery to see if it fixes it.

@pwolfert
Copy link
Collaborator

Another interesting thing I just found is that the current version of our SingleInputDatePicker doesn't work natively in a Preact project. It only works if Preact is in React-compatability mode. It works in examples/preact-react-app but not examples/preact-app.

@pwolfert
Copy link
Collaborator

pwolfert commented Mar 4, 2024

Noting here for context that upgrading to react-day-picker v8.10 is necessary to be able to upgrade date-fns to v3.0 so that that dependency supports ES Modules. Without that, modern build systems that need ES Modules support can't build a project that uses SingleInputDateField. Also note that we have a similar problem right now with the Tooltip component using a very old react-transition-group library without ES Modules support.

@pwolfert
Copy link
Collaborator

Putting this note here because it's related: If we wanted to be able to switch to the new JSX transform, I found the minimum requirements:

React 17 RC and higher supports it, but we’ve also released React 16.14.0, React 15.7.0, and React 0.14.10 for people who are still on the older major versions.

From here

@pwolfert
Copy link
Collaborator

Update: I've made a pull request to fix the issue upstream in react-day-picker

@pwolfert
Copy link
Collaborator

The pull request has been merged and released! I'm going to follow up on this tomorrow.

@pwolfert
Copy link
Collaborator

Succeeded by the pull request linked above

@pwolfert pwolfert closed this Apr 30, 2024
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

Successfully merging this pull request may close these issues.

2 participants