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

feat(remix-dev): make entry.{client,server}.tsx optional #4600

Merged
merged 63 commits into from
Feb 10, 2023

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Nov 14, 2022

made this before making a proposal like a dummy, but it's something we've talked about a bit internally

proposal here 👉 #4601

follow PR to make remix config optional 🎉

edit on the optional remix config.. turns out that already works as we check before we attempt to load it.

let configFile = findConfig(rootDirectory, "remix.config");
let appConfig: AppConfig = {};
if (configFile) {
let appConfigModule: any;
try {
// shout out to next
// https://github.com/vercel/next.js/blob/b15a976e11bf1dc867c241a4c1734757427d609c/packages/next/server/config.ts#L748-L765
if (process.env.NODE_ENV === "test") {
// dynamic import does not currently work inside of vm which
// jest relies on so we fall back to require for this case
// https://github.com/nodejs/node/issues/35889
appConfigModule = require(configFile);
} else {
appConfigModule = await import(pathToFileURL(configFile).href);
}
appConfig = appConfigModule?.default || appConfigModule;
} catch (error: unknown) {
throw new Error(
`Error loading Remix config at ${configFile}\n${String(error)}`
);
}
}

Signed-off-by: Logan McAnsh [email protected]

Closes: #

  • Docs
  • Tests

Testing Strategy:

@changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2022

🦋 Changeset detected

Latest commit: a7661fc

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

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

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 chore: make entry.{client,server}.tsx and root.tsx optional feat(remix-dev): make entry.{client,server}.tsx and root.tsx optional Nov 14, 2022
@MichaelDeBoey
Copy link
Member

What about people using JS instead of TS?
Do they need to have this file in their repo if they never changed it?

Should we also remove these files from our templates/examples?

@mcansh mcansh force-pushed the logan/dont-require-entry-files branch 2 times, most recently from 1a95227 to f531ea2 Compare November 15, 2022 15:54
@mcansh mcansh changed the title feat(remix-dev): make entry.{client,server}.tsx and root.tsx optional feat(remix-dev): make entry.{client,server}.tsx optional Dec 13, 2022
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 think we should also run the JS migration on the file when people choose the js/jsx extension in the generator command?

packages/remix-dev/cli/commands.ts Outdated Show resolved Hide resolved
packages/remix-dev/cli/commands.ts Outdated Show resolved Hide resolved
packages/remix-dev/cli/commands.ts Outdated Show resolved Hide resolved
packages/remix-dev/cli/commands.ts Outdated Show resolved Hide resolved
@mcansh
Copy link
Collaborator Author

mcansh commented Dec 15, 2022

I think we should also run the JS migration on the file when people choose the js/jsx extension in the generator command?

definitely, this is still very much WIP. it also needs to go through the RFC phase as the og proposal was sort of just thrown together

@mcansh mcansh force-pushed the logan/dont-require-entry-files branch from e98cd05 to 5e6c18c Compare December 21, 2022 17:42
@mcansh mcansh force-pushed the logan/dont-require-entry-files branch from 0f2b3f5 to 0c35245 Compare January 3, 2023 18:25
@mcansh mcansh marked this pull request as ready for review January 10, 2023 17:17
@mcansh
Copy link
Collaborator Author

mcansh commented Jan 10, 2023

What about people using JS instead of TS?

if they "eject" we'll transform it to js

Do they need to have this file in their repo if they never changed it?

doesn't hurt

Should we also remove these files from our templates/examples?

that's up for debate, templates would get any update faster than it being bundled in the CLI (unless we hit GitHub to get the file and use the bundled one if they're offline)

@mcansh mcansh force-pushed the logan/dont-require-entry-files branch from 028dde0 to 967a649 Compare January 17, 2023 20:34
Comment on lines 266 to 298
let runtime = deps["@remix-run/deno"]
? "deno"
: deps["@remix-run/cloudflare"]
? "cloudflare"
: deps["@remix-run/node"]
? "node"
: undefined;
Copy link
Member

Choose a reason for hiding this comment

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

This will stop working whenever we remove the explicit need of a runtime and we import everything from the adapter & separate server package (for functions like json & redirect)

Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
…r no ts; attempt to create both entries if no entry supplied

Signed-off-by: Logan McAnsh <[email protected]>
@mcansh mcansh force-pushed the logan/dont-require-entry-files branch from 9cec4e8 to a7661fc Compare February 10, 2023 19:16
@mcansh mcansh merged commit ecba23c into dev Feb 10, 2023
@mcansh mcansh deleted the logan/dont-require-entry-files branch February 10, 2023 19:38
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Feb 10, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-ecba23c-20230211 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.

🗺 Make entry.server and entry.client files optional
3 participants