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

[Bug]: createMemorySessionStorage doesn't actually work in development #1832

Closed
pckilgore opened this issue Feb 7, 2022 · 9 comments
Closed
Assignees
Labels
bug Something isn't working feat:sessions Sessions related issues needs-response We need a response from the original author about this issue/PR

Comments

@pckilgore
Copy link

pckilgore commented Feb 7, 2022

Which Remix packages are impacted?

  • remix (Remix core)
  • @remix-run/dev

What version of Remix are you using?

1.1.3

What version of Node are you using? Minimum supported version is 14.

16.13.0

Steps to Reproduce

use createMemorySessionStorage

Expected Behavior

sessions are preserved for the duration of the development session remix dev (or at least, preserved between changes to the underlying source).

Actual Behavior

in-memory session storage wiped after each request, even if no source code has changed.

Elaboration

I expected this API to be usable like:

export default process.env.NODE_ENV === "production"
    ? createCustomDBStore(cookie)
    : createMemorySessionStore(cookie)

Perhaps if this is not the intent and it is only intended for the case of "deployed development" environments, more clearly document that fact.

@pckilgore pckilgore added the bug Something isn't working label Feb 7, 2022
@pckilgore
Copy link
Author

Worked around in development with createFileSessionStorage

@akomm
Copy link

akomm commented Feb 8, 2022

When something in the file where you create the storage OR a file it depends on changes, it will be rebuild and reloaded. That might cause the wipe of the instance. I just guess because I only see the export, but it depends on cookie, so there is more, than just the default export, right? Can you check?

@confix
Copy link
Contributor

confix commented Feb 8, 2022

As of remix-serve

In development, remix-serve will ensure the latest code is run on each request by purging the require cache of your application modules when files change.

this might be what you are seeing.

#1818 implies that purging is happening per request and not when files have actually changed. I did not validate the assumption yet.

@pckilgore
Copy link
Author

pckilgore commented Feb 8, 2022

Purging appears to occur per request. It's reasonable to have the in-memory store refresh if a code change occurs.

But something logs me out between the response to the POST containing my session id and the GET to the authenticated route (which fails because the session associated with the ID is purged between requests). No code change required.

@akomm
Copy link

akomm commented Feb 9, 2022

You might stick and load the storage from global variable in development. But it might become stale and cause problems when you change things. As long as dependencies don't change in an incompatible way, this includes also configuration from environment set once on instantiation, there should be no problem.

Example:

// utils/xy.ts or utils/xy.server.ts
export const y = Math.random()

export const x = (() => {
  if ("development" !== process.env.NODE_ENV) {
    return Math.random()
  }

  // names on global should be more distinct...
  const g = global as {x?: number}
  const x = g.x

  if (typeof x !== "number") {
    // Math.random() is your service instantiation
    // If "x" has some dependencies on "a" and "b" (primitive types) you need to track it via global too for changes
    // if (typeof x !== "number" || a !== g?.xOptions?.a || b !== g?.xOptions?.b)  /* recreate both xOptions and x on global */
    return (g.x = Math.random())
  }

  return x
})()
// some route
import {LoaderFunction} from "remix"
import {x, y} from "~/utils/xy.server.ts"

type LoaderData = {x: number; y: number}
export const loader: LoaderFunction = () => json<LoaderData>({x, y})

export default function SomeRoute() {
    const {x, y} = useLoaderData<LoaderData>()
    return (<div>x: {x}, y: {y}</div>)
}

@itacirgabral
Copy link

funny warning:

This should only be used in development (...)
https://remix.run/docs/en/v1/utils/sessions#creatememorysessionstorage

But it only works in prod :s

maybe this code above could be workarounded like the prisma hack
https://remix.run/docs/en/v1/tutorials/jokes#connect-to-the-database

type SessionFactoryMap = Map<string, {
    data: SessionData;
    expires?: Date | undefined;
}>
declare global {
  var __sfm: SessionFactoryMap | undefined;
}

let map: SessionFactoryMap

if (process.env.NODE_ENV === "production") {
  map = new Map<string, { data: SessionData; expires?: Date }>();
} else {
  if (!global.__sfm) {
    global.__sfm = new Map<string, { data: SessionData; expires?: Date }>();
  }
  map = global.__sfm;
}

@akomm
Copy link

akomm commented Jan 31, 2023

This is basically my proposal above, but without the notice to track the dependencies. Many copy the workaround proposed for prisma, without realizing that in the other case their instantiation has parameters (dependencies) which might change. If they want them to update, they need to track them in global as well. Otherwise you need to restart the server.

It is not the restart that is a problem, but people confused why something doesn't work, but its simply because they still run with old options passed to the instance.

@brophdawg11 brophdawg11 added the feat:sessions Sessions related issues label May 5, 2023
@brophdawg11 brophdawg11 self-assigned this Aug 3, 2023
@brophdawg11 brophdawg11 added this to v2 Aug 3, 2023
@brophdawg11 brophdawg11 assigned mjackson and unassigned brophdawg11 Aug 3, 2023
@brophdawg11 brophdawg11 moved this to Backlog in v2 Aug 3, 2023
@mjackson
Copy link
Member

This shouldn't be a problem with the v2_dev server since we don't purge modules after every request anymore. With v2_dev, if you don't change your app code, in-memory stuff just works ootb.

Please give this a shot with the v2_dev flag and let us know how it goes 🙏 Thanks!

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Aug 15, 2023
@github-actions
Copy link
Contributor

This issue has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from issues that are unactionable. Please reach out if you have more information for us! 🙂

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 26, 2023
@MichaelDeBoey MichaelDeBoey moved this from Backlog to Closed in v2 Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feat:sessions Sessions related issues needs-response We need a response from the original author about this issue/PR
Projects
No open projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

7 participants