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

remix package reorg #9034

Closed
wants to merge 2 commits into from
Closed

remix package reorg #9034

wants to merge 2 commits into from

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Mar 11, 2024

Work can happen in phases that could be separate PRs / separate releases, but can also do all of this in fewer or even a single PR if we want.

Phase 1

  • Copy remix-server-runtime into remix pkg
  • Remove exports from factory functions in remix pkg

If we want, we can move onto phase 2 as part of this PR. Or we can start releasing and make sure that remix pkg works as expected everywhere, temporarily having some duplication b/w remix and remix-server-runtime.

Phase 2

  • Runtime pkgs (@remix-run/{cloudflare,deno,node}) re-export from remix instead of remix-serve-runtime
  • Back compat: remix-server-runtime re-exports from remix instead of providing its own implementations
  • Implement cookie/session functions in remix pkg using web standard crypto
  • Update docs / templates to use remix pkg

Phase 3

  • Deprecate remix-server-runtime and its exports
  • Codemods (?)

Copy link

changeset-bot bot commented Mar 11, 2024

⚠️ No Changeset found

Latest commit: 3fcabd7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@brophdawg11
Copy link
Contributor

I'm unsure if we'll run into merge conflicts with changes made in server-runtime (thinking of single fetch primarily at the moment) but if so could we consider a phase 0 where the remix package just re-exports from code in server-runtime? Then we could move implementations over to remix as needed (a few files at a time or all at once) and re-export from server-runtime. That would avoid any duplication in the dev branch and also avoid any merge conflicts.

I'm hoping to merge single fetch to dev today so we should have an idea on the merge conflicts soon and if they're note an issue then ignore this :)

My vote would be to try to avoid dup implementations though however we go about it

@pcattori
Copy link
Contributor Author

I'm unsure if we'll run into merge conflicts with changes made in server-runtime (thinking of single fetch primarily at the moment) but if so could we consider a phase 0 where the remix package just re-exports from code in server-runtime? Then we could move implementations over to remix as needed (a few files at a time or all at once) and re-export from server-runtime. That would avoid any duplication in the dev branch and also avoid any merge conflicts.

I'm hoping to merge single fetch to dev today so we should have an idea on the merge conflicts soon and if they're note an issue then ignore this :)

My vote would be to try to avoid dup implementations though however we go about it

The issue is that establishing remix-server-runtime as a dependency of remix means that we can't incrementally migrate to remix as a dependency of remix-server-runtime, which is where we want to end up.

Another alternative I considered is creating a (new) 3rd package that both remix and remix-server-runtime depend on, and then incrementally moving parts of remix-server-runtime into that new package. Then we can incrementally switch thing from that new package into remix. But this seemed needlessly complex.

My proposal is to "fork" remix-server-runtime into remix, only land new changes in remix, and phase out remix-server-runtime.

@brophdawg11
Copy link
Contributor

Ah good point, that would mean a circular dependency between the two. And I agree the 3rd package seems overly complex.

I don't like the idea of duplicate implementations in dev though - it seems like an invitation for confusion on devs part (us and the community) potentially making changes to the wrong implementation. And then we also have an integration test concern - do we go through every test and change it to import from remix leaving the implementations in server-runtime untested?

I would vote to do the fork but also include this this step into Phase 1:

Back compat: remix-server-runtime re-exports from remix instead of providing its own implementations

Would that just involve something like export * from 'remix' to a server-runtime/index.ts file?

If we do that, we never have dup implementations in dev, and hopefully Git is then even able to detect the straight file moves and handle changes correctly when merging dev into existing PR branches in many cases.

@pcattori pcattori closed this Apr 17, 2024
@pcattori pcattori deleted the pedro/pkg-reorg branch April 17, 2024 16:51
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.

2 participants