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(remix-react): Remove nullish coalescing operator #4561

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

machour
Copy link
Collaborator

@machour machour commented Nov 10, 2022

Closes: #

  • Docs
  • Tests

Testing Strategy:

I was testing my app with an iPhone 6, and found out the client side was totally broken.

We had a discussion with @sergiodxa a few days ago where we uncovered that we re-introduced a nullish coalescing operator ?? in #3697 that could be problematic for old browsers.

I can now confirm that it is indeed problematic. 🙃
This PR gets rid of it.

@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2022

🦋 Changeset detected

Latest commit: 08c8848

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/react Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/vercel Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

I don't think we should make this change like this tbh.
We're supporting Node v14+, so nullish coalescing should be possible to write without any problems.

In case of the older browser being broken, this is a matter of broken transpiling, most likely not taking these versions into account in browserslist

@machour
Copy link
Collaborator Author

machour commented Nov 10, 2022

Will happily update the PR if there's a better way to do it. Could you set me on the right track to fix the transpilling part?

@MichaelDeBoey
Copy link
Member

@machour I think I once saw a discussion about supporting a browserslist config, because (if I correctly remember) we're currently only supporting the defaults provided by esbuild

@machour
Copy link
Collaborator Author

machour commented Nov 10, 2022

Gotcha. Leaving this as it is for the meantime. It was working, it's not anymore, so we may want to fix this first, then look at changing the way we do things.

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Nov 10, 2022

If this only changed recently, this can be a bug introduced when @pcattori extracted browser+server compiler out of the massive compiler we had before. 🤔

Could be a totally different thing as well, I just know that we had some work done there 🤷‍♂️

Copy link
Member

@sergiodxa sergiodxa left a comment

Choose a reason for hiding this comment

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

Until browserlist is supported I think this one line change it’s simpler and can help a lot of people support older browsers with ESM support

@chaance
Copy link
Collaborator

chaance commented Nov 10, 2022

I'm going to merge this for now, but we'll prioritize a strategy for dealing with this to prevent regressions over the next week.

@chaance chaance merged commit 02f8fa0 into remix-run:dev Nov 10, 2022
@pcattori
Copy link
Contributor

If this only changed recently, this can be a bug introduced when @pcattori extracted browser+server compiler out of the massive compiler we had before. 🤔

The compiler code never makes it into the browser, so nullish coalescing is fine in compiler / server-only code.
In this case, its in useFormAction, which does make it into the browser.

I agree that we should allow Node v14 features like nullish coalescing in our codebase and just transpile them away for the browser build.

@machour machour deleted the fix/old-browsers-support branch November 10, 2022 15:59
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-929d847-20221111 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants