Closed ryanflorence closed 1 year ago
I strongly disagree with this change.
The React DevTools will be much more difficult to use if every route is called "Component". This is one situation where a default export is very useful.
I'm also not a fan of this change. I think this is one of the very few cases where a default export is useful, allowing libraries/frameworks to give users the option to name an export however they want.
It also encourage exporting anonymous components.
Most projects use ESLint, and there's already a rule that can prevent this: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-anonymous-default-export.md.
import { Component, loader, action } from "~/routes/something" import { render } from "@testing-library/remix" // fake lib, for now π test("Component renders", async () => { render(
) // more test code here })
Seems like the idea is to keep every test file consistent. This is also doable with
import Component, { loader, action } from '~/routes/something'
import { render } from "@testing-library/remix" // fake lib, for now π
test("Component renders", async () => {
render(<Component />)
// more test code here
})
I'm in favor. The pros outweigh the cons.
It's the only route feature that doesn't follow the pattern.
Plus:
import { Component as IndexRoute } from "~/routes/index"
can still be used in exceptional scenariosReact Dev Tools
It's true that we'll lose a bit of organization here, especially when using nested routes we'll start getting Component1
, Component2
, etc.
However, I don't think it's that important because:
RemixRoute
or RemixCatchBoundary
, and as the only child.IMHO, this being a pure DX change, all pros/cons are and will be biased by personal taste π°
All in all, it would enhance tests DX while degrading app DX (devtools).
Sounds to me like an unneeded breaking change, and even with a major version bump, we want to have as little as possible of those)
While it's technically not breaking because either option works, I'm against the idea of adding more ways to do the same thing. It just adds confusion.
I'd be in favor of a breaking change here (with a codemod) if the Remix compiler adds a display name that will show the route name in the DevTools.
if the Remix compiler adds a display name that will show the route name in the DevTools.
This is my only concern with the Component
naming here. I don't spend much time in the React DevTools, but when I do... seeing Component listed many many times isn't helpful. Besides that, I'm ready for Component
.
Just going to drop a link to my thoughts in the other discussion to explain why I am opposed to this. My opinion mirrors much of what @kentcdodds has already said, but I wanted to make sure it was on the record π https://github.com/remix-run/remix/discussions/4719#discussioncomment-4276919
To clarify: I'm not opposed to this because I like defaut exports. To me this is like arguing over let vs. const or semicolons. I literally couldn't care less. I am opposed because, in order to accommodate this we'd either:
I'm generally getting uncomfortable with how many things we're doing that will require refactoring to upgrade in a couple of months. And unless we can articulate a clear benefit (which I don't feel like we've done here), I really think we should avoid it as much as possible.
ALL THAT BEING SAID: If I've already lost this battle and this proposal is a foregone conclusion, I'd like to cast a strong vote for option 1 and avoid the introduction of a breaking change. I don't love muddying up simple conventions, but in this case it seems easy enough to do and it'll avoid a lot of migration headaches.
To solve the devtools issue, the route loader could give the component a displayName, something like this:
for (const route of loadedRoutes) {
route.component.displayName = `Route(${route.path})`
}
I recognize this may look like a superfluous change, so hear me out.
If we want a framework we love in 5 years from now, I think we have to make these small adjustments, too.
When I recently rearranged the menu in the Remix docs, it took a bit of thought about what to call the "default export" and what the URL to the doc should be. Here are the sibling docs and their paths:
So what would you put in the menu and what would the path be for the default export of a route module?
I tried a few things:
Component seems like the obvious choice, it's how we talk about it already: "your route component".
I prefer when "talking about code" and "writing code" line up.
Nobody is saying "default export" when talking about their "route component". All of the docs say "route component", all of the community blog posts and tutorials say "route component", and all of the future resources will say "route component".
Let's line up the API with the vocabulary, especially since the upgrade path is a quick remix codemod route-component
away.
I'm convinced. Let's just make sure the display name is handled and there's only one way to do it (eventually) and I'll be happy.
@ryanflorence I hear what you're saying. As a PHP/WordPress alum, I have just always appreciated the commitment to back compat and truly believe that's a core pillar of what makes that space so strong and successful (heck, even React is pretty great on this front). I'd like for us to have some sort of way to evaluate breaking changes specifically to make sure the juice is worth the squeeze. Even then, we should try to provide a steady off-ramp for our usersβeven if that means holding onto some duplication and maintenance version for a bit.
Though I'm still not entirely convinced on the merits of this one, can we at least offer deprecation warnings for the next major release before completely pulling the rug out from users? Seems like a good rule-of-thumb for most breaking changes. We own it for at least one major release to give folks more time to migrate to new conventions while still benefitting from new features (this could probably be it's own proposal).
I remember using early versions of Next and going through the Now v1 > v2 upgrade and it was painful, and there weren't many options to avoid it at the time. This means user-facing priorities got the backseat to internal upgrades. The team was not happy and started questioning our stack choice.
can we at least offer deprecation warnings for the next major release before completely pulling the rug out from users
That's always the case when changing APIs. Opt-in flag or dual compatibility in v1, deprecation warnings before v2, removal in v2.
That's always the case when changing APIs. Opt-in flag or dual compatibility in v1, deprecation warnings before v2, removal in v2.
Right, I'm suggesting an opt-in flag or dual compatibility in v1, rename flag in v2 to clarify the feature is deprecated (plus deprecation warnings) in v2, removal in v3. Especially if we are releasing v2 soon after making the change, which we are at this point.
(starting to convince myself that we should do this with meta
as well)
I agree with @chaance on the keeping default on v2 and only remove it on v3.
Every deprecated feature should be usable for one major version behind a flag to give more time for migration, plus a code-mod to do it automatically.
And yes, meta
should follow the same pattern, keep meta_v2 behind a flag in Remix v1, swap the flags in Remix v2 so meta_v1 is default and meta_v1 is optional, remove meta_v1 in Remix v3
I understand is more code to maintain but it will help make the migrations easier, if you can do the same with flat routes would be ideal, so any Remix v1 app could upgrade to Remix v2 and just enable some flags and get deprecation warnings.
I think the feature-by-feature opt-in of breaking changes in v1 is enough to make the migration path much better than what most people experience with most other packages (as noted with Next v1 to v2). Combine that with a codemod and I just really see no point in Remix shouldering the burden of dual support in v2. People will have plenty of time to upgrade feature-by-feature and with a codemod for stuff like this in particular, the migration for this specific feature should take a few minutes tops π€·ββοΈ
Lets keep this conversation to whether or not export Component
is a net good change to make rather than how we move the API forward incrementally without breaking apps (which we do and will continue to do!)
Just dumping some of my thoughts on this here:
.displayName
for dev tools now, something we don't have to do today and I'm unlikely to take up as a practiceAddressing documentation for the default export:
We are going to support multiple frameworks, so name it /route/react
. We will need a page for each framework in the future anyways. Also, what happens when the framework doesn't call them "components", and instead "elements", or the next word that's invented? Does that framework's remix integration's module format change from Component
to Element
as the export? Keeping it default allows us to remain future proof in this sense. Your "view" is the default export, and when referring to adding something to the default export component, just refer to it as "The View", or "The View Component".
I'd also like to respond to a few other comments above:
helper()
/helpers.ts
, but it breaks my brain more than most probably.import {Component as TheActualDescriptiveName}
, this is not an exceptional scenario.My takeaways:
I think we haven't been clear. Nobody is saying you have to add .displayName
manually. This would be done by Remix at compile-time. So you'd just do:
// app/routes/search.tsx
export function Component() {
/// yada
}
And the compiler would output something like:
export function Component() {
/// yada
}
Component.displayName = 'SearchRoute'
Or something like that.
I think we haven't been clear. Nobody is saying you have to add
.displayName
manually. This would be done by Remix at compile-time. So you'd just do:// app/routes/search.tsx export function Component() { /// yada }
And the compiler would output something like:
export function Component() { /// yada } Component.displayName = 'SearchRoute'
Or something like that.
I'm aware, but We could instead keep it default, and add YourDescriptiveComponentName (routes/the-file)
. It's not one or the other for the "enhance the debug experience", but I'd even go further and say even if we do name them automatically for the Component
export, it is still a degraded debug / authoring experience.
I haven't seen a good reason to do this.
Your "view" is the default export, and when referring to adding something to the default export component, just refer to it as "The View", or "The View Component".
I think when we support multiple frameworks we'd use the right word for the framework? export View
?
But yeah, I see the value in keeping it as default for this reason so that the route module API (and compiler that removes server code) doesn't care about the rendering lib. Only the default export can be used differently for rendering libs, but loader/action/etc. would be the same everywhere.
First off, I just wanna thank everyone for weighing in here. It's important that we carefully consider changes we are making to the framework as we move it forward.
I agree with Ryan here that the semantics matter, i.e. it's nice when the way we talk about the code and the words we use in the actual code are aligned.
Everything in a route module has a descriptive name. Your loader
, action
, headers
, ErrorBoundary
... even your default
! Er ... wait. What exactly does the default
export do? Oh right, that's your component!
So as for "this change would bring no meaningful improvement to Remix" I'd have to disagree. Aligning our code with the language we use in regular conversation and documentation is a worthwhile goal.
Imagine you're helping someone who is new to Remix debug an issue in one of their routes that isn't displaying correctly. You might say "ok, first let's check the loader to see it's returning the data we expect. Uh huh. Ok, that looks good. Now what does the component look like?" At that point they might be confused. Where's the component? At which point you'd say "it's the default export" or something similar.
This change would prevent any confusion from ever happening in the first place, which is a meaningful improvement! π
Again, I thank and respect the opinion of everyone who has participated here. Thank you! But having read all comments up to this point I'm still inclined to do this.
I like the idea of a conventionally-named export, but also don't think my opinion is particularly relevant :P
That being said...if the framework moves away from the default
export, does that mean a new convention will be needed for creating resource routes? Or would the rule change to
If a route doesn't export a
Component
component (?), it can be used as a Resource Route
Separately - if y'all move forward w/ no more default
export, would a more agnostic name like Main
(if not View
) help with the nomenclature differences between frameworks?
If you can detect that a default export is anonymous, in development mode, I think it is better to throw an error to force the user to have a named default export.
I just follow the pattern:
export const loader = async () => { /* ... */ }
export const MyNamedComponent = () => { /* ... */ }
export default MyNamedComponent
In VSCode I have a user defined snippet 'rmxroute' which will create all and name the component the same as the file name.
You could even rename your loader/action/whatever export to make it even easier to import into your tests export { myNamedComponentsLoader: loader }
Imagine you're helping someone who is new to Remix debug an issue in one of their routes that isn't displaying correctly. You might say "ok, first let's check the loader to see it's returning the data we expect. Uh huh. Ok, that looks good. Now what does the component look like?" At that point they might be confused. Where's the component? At which point you'd say "it's the default export" or something similar.
This change would prevent any confusion from ever happening in the first place, which is a meaningful improvement! π
Having onboarded quite a few developers to a few Remix apps (even developers with little JS experience), I can't say that I agree with this argument. The name "component" does not have a strong meaning, people have related more to "layout of the route" than anything else, so I'd argue that for the teaching aspect of this, "Component" isn't even a good name.
Everything in a route module has a descriptive name. Your
loader
,action
,headers
,ErrorBoundary
... even yourdefault
! Er ... wait. What exactly does thedefault
export do? Oh right, that's your component!So as for "this change would bring no meaningful improvement to Remix" I'd have to disagree. Aligning our code with the language we use in regular conversation and documentation is a worthwhile goal.
And related to my previous point, in regular conversations "Component" isn't the name people tend to refer.
Disclaimer: none of the people I've onboarded had any experience with React Router, so they might be biased (or maybe "unbiased"?). But since Remix tends to align things with React Router, I understand why you'd reach for this name, unfortunately.
I'm with Jacob on this subject, and would be quite upset to see this change.
@duailibe It sounds that maintaining default
is not a better solution than Component
. It is rather having a more descriptive (framework-agnostic) name.
@sergiocarneiro I could argue that maintaining default
is not a worse solution than Component
either, but it's the status quo and won't break anything.
I'll give you that it's not super descriptive (which, in my anecdotal experience, is not really a problem), though it's very framework-agnostic.
Closing this, no need to churn the API here even if I like it better π
Discussed in https://github.com/remix-run/remix/discussions/4719