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

fix(crwa): Explicit check for possible null value in entry.client.tsx #9251

Merged
merged 6 commits into from
Oct 10, 2023

Conversation

Josh-Walker-GM
Copy link
Collaborator

Problem
Fixes #9059 which is a type error raised when you have strict type checking enabled. The getElementById call can return null as the MDN docs mention.

Changes
Adds a basic null check taken from the solution given in #9059.

Considerations
This adds some boilerplate code to a user facing file but I think it's a reasonable addition. It's readable and clear what the code is doing without the need for further explanation or technical knowledge. In the case where this causes a runtime error then the message is clearer:
Screenshot from 2023-10-02 23-16-38

I haven't immediately been able to think of some hidden solution to this that isn't exposed to the user in some boilerplate code.

@Josh-Walker-GM Josh-Walker-GM added the release:fix This PR is a fix label Oct 2, 2023
@Josh-Walker-GM Josh-Walker-GM added this to the next-release-patch milestone Oct 2, 2023
@Josh-Walker-GM
Copy link
Collaborator Author

@jtoar - Do you think this one needs a larger team/group agreement or could we just get this in like any standard change?

@Tobbe
Copy link
Member

Tobbe commented Oct 3, 2023

I'd do if (!redw... but other than that it LGTM

@Josh-Walker-GM
Copy link
Collaborator Author

Happy to change it to the falsy condition

@Josh-Walker-GM
Copy link
Collaborator Author

Josh-Walker-GM commented Oct 3, 2023

I guess this would actually have to be a manual change for existing projects. Do we need to consider a codemod here or is the change simple enough to document? It's only encountered when you opt-in to strict types so there is already a manual barrier in that regard.

Edit: I'll add a codemod for this. Please wait for that addition before merging.

@Josh-Walker-GM Josh-Walker-GM requested a review from jtoar October 5, 2023 01:28
@jtoar jtoar modified the milestones: next-release-patch, v6.3.2 Oct 5, 2023
@Josh-Walker-GM
Copy link
Collaborator Author

Added the complementary codemod for this change. It should easily cover the vast majority of users - only those who have customised the entry.client.tsx file with things like variable renaming should have to implement the change manually. It's not a difficult change to implement anyway.

Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Josh-Walker-GM!

@jtoar jtoar merged commit b0964a9 into main Oct 10, 2023
30 checks passed
@jtoar jtoar deleted the jgmw-crwa/entry-client-fix branch October 10, 2023 07:12
jtoar pushed a commit that referenced this pull request Oct 10, 2023
…x` (#9251)

**Problem**
Fixes #9059 which is a type error raised when you have strict type
checking enabled. The `getElementById` call can return null as the [MDN
docs](https://developer.mozilla.org/en-US/docs/Web/API/Document/getElementById#return_value)
mention.

**Changes**
Adds a basic null check taken from the solution given in #9059. 

**Considerations**
This adds some boilerplate code to a user facing file but I think it's a
reasonable addition. It's readable and clear what the code is doing
without the need for further explanation or technical knowledge. In the
case where this causes a runtime error then the message is clearer:
![Screenshot from 2023-10-02
23-16-38](https://github.com/redwoodjs/redwood/assets/56300765/ca6d51bd-2c28-438e-87d3-7411e54721ec)

I haven't immediately been able to think of some hidden solution to this
that isn't exposed to the user in some boilerplate code.
@jtoar jtoar modified the milestones: next-release-patch, v6.3.2 Oct 10, 2023
jtoar pushed a commit that referenced this pull request Oct 10, 2023
…x` (#9251)

**Problem**
Fixes #9059 which is a type error raised when you have strict type
checking enabled. The `getElementById` call can return null as the [MDN
docs](https://developer.mozilla.org/en-US/docs/Web/API/Document/getElementById#return_value)
mention.

**Changes**
Adds a basic null check taken from the solution given in #9059. 

**Considerations**
This adds some boilerplate code to a user facing file but I think it's a
reasonable addition. It's readable and clear what the code is doing
without the need for further explanation or technical knowledge. In the
case where this causes a runtime error then the message is clearer:
![Screenshot from 2023-10-02
23-16-38](https://github.com/redwoodjs/redwood/assets/56300765/ca6d51bd-2c28-438e-87d3-7411e54721ec)

I haven't immediately been able to think of some hidden solution to this
that isn't exposed to the user in some boilerplate code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Generated entry.client.tsx isn't passing rw type-check
3 participants