-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add remix package #10894
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
base: main
Are you sure you want to change the base?
Add remix package #10894
Conversation
mjackson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks pretty good. The barrel files (index.ts and index-platform.ts) will still need some work to be complete, but we can continue to add things they are missing over time as we begin to use the remix package in earnest. This is a good start.
packages/remix/src/index-platform.ts
Outdated
| // All other APIs based on core Javascript/Web Fetch API should be exported | ||
| // from the root remix package in `index.ts` | ||
|
|
||
| export { asyncContext, getContext } from './lib/async-context-middleware' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note on import/export paths from files (not packages): we always use the file extension to avoid ambiguity. So this should be:
| export { asyncContext, getContext } from './lib/async-context-middleware' | |
| export { asyncContext, getContext } from './lib/async-context-middleware.ts' |
Maybe we want to add an eslint rule that catches this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting - I thought that would already fail at build time and didn't even notice the LLM left them out. I'll look into a way to catch this automatically 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a lint rule in 625b639
| get, | ||
| post, | ||
| put, | ||
| resources, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have resource here as well, along with head, options, and patch. I'm thinking we should also have the longform names, createGetRoute, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
demos/bookstore/app/utils/context.ts
Outdated
| import { createStorageKey } from '@remix-run/fetch-router' | ||
| import { getContext } from '@remix-run/async-context-middleware' | ||
| import { createStorageKey } from 'remix' | ||
| import { getContext } from 'remix/platform' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something feels off to me about this. I have to know that getContext relies on node:async_hooks internally so that I know to get it from remix/platform. Maybe it's better to just import directly from remix/async-context-middleware and ditch the whole idea of remix/platform entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah - I think thats a good call. The sub-export is just an added layer of indirection when it's still a single dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import type { Middleware, Route } from '@remix-run/fetch-router' | ||
| import { createRedirectResponse as redirect } from '@remix-run/response/redirect' | ||
| import type { Middleware, Route } from 'remix' | ||
| import { createRedirectResponse as redirect } from 'remix' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is so common we'll probably want to have a redirect shorthand that people can import instead of renaming it every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should do the same for createHtmlResponse and createFileResponse? html would conflict at a global level with the html string helper so if we chose to export from the top-level we'd have to choose one, but they'd both be fine as exports from their individual sub-exports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the html helper. I want to see how it feels once we start using components more heavily in the demos and relying less on the html-template library. I just know for now that I want to be able to import redirect directly, or I'm always going to do this rename.
12b0f08 to
9b91b90
Compare
8c79a0f to
f0f2f14
Compare
mjackson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I'm starting to think we don't want as much stuff in the main remix export as we originally thought. I realize it feels subjective at this point, but we knew this would be a somewhat subjective exercise going into it.
| import type { Middleware, Route } from '@remix-run/fetch-router' | ||
| import { createRedirectResponse as redirect } from '@remix-run/response/redirect' | ||
| import type { Middleware, Route } from 'remix' | ||
| import { createRedirectResponse as redirect } from 'remix' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the html helper. I want to see how it feels once we start using components more heavily in the demos and relying less on the html-template library. I just know for now that I want to be able to import redirect directly, or I'm always going to do this rename.
Stacked on #10888
Adds a top level
remixpackage that is auto-generated as an umbrella package over all@remix-run/*packages in the repo.export * from '@remix-run/...';files inpackages/remix/src/lib@remix-run/file-storage->remix/file-storage@remix-run/file-storage/fs->remix/file-storage/fspnpm generate-remixwill generate the remix package based on the current state of the repomainremix/src/index.tsfile is manually authored to control the APIs exported from the top-levelremixpackagedemos/, but I'm sure we'll want to add more as we start taking a closer look at the entire API surface and building more demosindex-client.tsfile that exports a subset of theremixAPI fromindex.tsthat can run in the browser - eliminating anything that imports fromnode:*under the hoodclientcondition so you can build for the browser withesbuild --conditions=clientwithout running across any node imports that would otherwise fail the builddemos/have been converted to use the top-level package and remove@remix-run/*packages from theirdependencies