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

refactor(remix-server-runtime): expose SessionStorage's methods as arrow functions #6330

Merged
merged 6 commits into from
May 15, 2023

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented May 6, 2023

Summary of changes

This is just the relevant parts of the code changes from #6325 instead of also tackling eslint-config changes.

  • Expose methods in the SessionStorage interface as arrow functions so destructuring is correctly part of the contract.

Details

I attempted to add the typescript-eslint rules contained in "plugin:@typescript-eslint/recommended-requiring-type-checking" to a Remix based project and came across two common code issues:

const { pipe, abort } = renderToPipeableStream();

and then on:

const { getSession, commitSession, destroySession } = createDatabaseSessionStorage();

Both of these cause an unbound-method error:

error Avoid referencing unbound methods which may cause unintentional scoping of this.

If your function does not access this, you can annotate it with this: void, or consider using an arrow function instead @typescript-eslint/unbound-method

This error needed to be taken care of by the type definitions provided by the libraries.

@types/react-dom v18.2.4 now includes this: void in the functions returned from renderToPipeableStream(), so the first half is solved.

This pull request is to address the changes needed in Remix.

This could subtly change the contract for external libraries returning SessionStorage, so it would be good to get this in before Remix v2.

Testing Strategy:

I opened up my windows machine and ran this script:

yarn test:primary
npx playwrite install
yarn test
yarn build --tsc

Also tested on a Remix project with eslint and "plugin:@typescript-eslint/recommended-requiring-type-checking" enabled.

error  Avoid referencing unbound methods which may cause unintentional scoping of `this`.

If your function does not access `this`, you can annotate it with `this: void`, or consider using an arrow function instead @typescript-eslint/unbound-method
@changeset-bot
Copy link

changeset-bot bot commented May 6, 2023

🦋 Changeset detected

Latest commit: 45ac70c

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

This PR includes changesets to release 18 packages
Name Type
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
create-remix Patch
@remix-run/css-bundle Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/serve Patch
@remix-run/testing Patch
@remix-run/vercel Patch
remix Patch
@remix-run/eslint-config 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

@MichaelDeBoey MichaelDeBoey changed the title Fix: add this: void to session methods that are commonly destructured refactor(remix-server-runtime): add this: void to methods that are commonly destructured May 6, 2023
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.

Let's use arrow functions instead, as @JoshuaKGoldberg suggested in #6330 (comment)

packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
packages/remix-server-runtime/sessions.ts Outdated Show resolved Hide resolved
.changeset/polite-garlics-invite.md Outdated Show resolved Hide resolved
@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 6, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 6, 2023
@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 6, 2023
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented May 6, 2023

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 6, 2023
@ngbrown ngbrown changed the title refactor(remix-server-runtime): add this: void to methods that are commonly destructured refactor(remix-server-runtime): expose SessionStorage methods as arrow functions May 6, 2023
@ngbrown ngbrown requested a review from MichaelDeBoey May 6, 2023 20:51
@ngbrown ngbrown force-pushed the session-destructured-methods branch from 1c59001 to 45ac70c Compare May 6, 2023 20:58
@MichaelDeBoey MichaelDeBoey changed the title refactor(remix-server-runtime): expose SessionStorage methods as arrow functions refactor(remix-server-runtime): expose SessionStorage's methods as arrow functions May 6, 2023
@brophdawg11 brophdawg11 merged commit dcd7c2e into remix-run:dev May 15, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-360e77e-20230516 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!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.16.1-pre.0 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!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.16.1 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.

4 participants