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

Feature: add default export option when using a schematic to generate a component #25023

Closed
brandonroberts opened this issue Apr 14, 2023 · 14 comments · Fixed by #28268
Closed
Labels
area: @schematics/angular feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature good first issue

Comments

@brandonroberts
Copy link
Contributor

brandonroberts commented Apr 14, 2023

Command

generate

Description

When I want to generate a component to use as a lazy loaded component with loadComponent I can use the schematic

ng generate component about --standalone
```ts
import { Component } from '@angular/core';
import { CommonModule } from '@angular/common';

@Component({
  selector: 'app-about',
  standalone: true,
  imports: [CommonModule],
  templateUrl: './about.component.html',
  styleUrls: ['./about.component.css']
})
export class AboutComponent {

}

Then when I go to use the component in the route config, I immediately get an error because its not a default export

[
  { path: 'about', loadComponent: () => import('./about/about.component') }
]

Describe the solution you'd like

Add an option to generate a component with the class as a default export

ng generate component about --standalone (--export-default or --default-export)
import { Component } from '@angular/core';
import { CommonModule } from '@angular/common';

@Component({
  selector: 'app-about',
  standalone: true,
  imports: [CommonModule],
  templateUrl: './about.component.html',
  styleUrls: ['./about.component.css']
})
export default class AboutComponent {

}

Describe alternatives you've considered

Manually update the component after generation

@clydin clydin added feature Issue that requests a new feature area: @schematics/angular labels Apr 14, 2023
@alan-agius4
Copy link
Collaborator

//cc @dgp1130 as he had some strong opinions about default exports in general as they go against G3 guidelines.

@dgp1130
Copy link
Collaborator

dgp1130 commented Apr 14, 2023

The google3 TypeScript style guide generally disallows default exports and provides some justification. I think all of those points generally apply outside of Google as well. Angular style guide doesn't seem to provide any guidance here, but it does include the line "Do define one thing, such as a service or component, per file" with one reasoning for it being "A single component can be the default export for its file which facilitates lazy loading with the router", which at least tells me that default exports aren't disallowed.

Additionally external to google3 we have CommonJS interop, which is a major challenge for default exports. These days, Angular is all-in on ESM so that isn't a major issue, but if devs choose to use default exports in their application, it easily bleeds over into other CommonJS libraries they might be using and introduce those incompatibilities, so I still generally would not recommend default exports, solely for that reason.

That said, personally I'm ok with default exports when they avoid magic names. For example, if we had an API like loadComponent('foo') and under the hood that did import('foo').then((module) => doSomethingWith(module.someExport)), then I feel a default export is actually simplifying that experience and avoiding a requirement for the magic someExport name with special behavior. Of course we've generally avoided those kinds of designs for a number of reasons, the router being one notable area.

Recently we did add support for default exports in the router, so I can understand wanting to take advantage of that out of the box when generating a component. I can see some value to the idea that "all route components should use default exports".

However the core philosophical concern I have is that defining a component is independent of defining a route. The component decides whether or not it is the default export, yet the route is the one which is simplified by consuming a default exported component. It is entirely possible to have a component used as a route in one location, but then also used as a sub-component in another location, yet this presents a contradiction between "all route components should use default exports" and "all non-routes should use named exports", since the component is used in both contexts.

This makes me think that ng generate component is the wrong place for this kind of functionality. If we have some reasonable guarantee that a component is always used as a route (such as a home.route.ts file convention or a routes/ directory), then a default export might make sense. However ng generate component doesn't know that.

We've discussed adding ng generate route for standalone, which I think would generate the underlying component as well. I wonder if it would make more sense to use a default export there (whether by default or as an option) since it is very likely that this component will only be used as a route. I guess it might need to be plumbed through to the component schematic anyways to support that, but I think it would present a better experience if users don't need to think about it. I know I would never think to write --default-export in advance and would just find it easier to add the default keyword manually. But if we made ng generate route use a default export by default, users would not need to think about it or explicitly make that choice. Of course the trade-off here is that users might not understand why some components use default exports and why some don't, which can lead to general confusion.

I'm curious to hear @atscott, @alxhub, and @jelbourn's opinions on this. I think a lot of it comes down to:

  1. Do we want or intend to be more restrictive about default export in the Angular style guide?
  2. Do we want to encourage or recommend default export over named exports for routing use cases?
    a. If yes, are we ok with diverging from the Google style guide here? Should we explore making the same exception in the google3 style guide?
  3. Does this fit in ng generate component? Does it fit in ng generate route instead?

Ultimately, like most things with schematics this comes down to a style question, and the answer to that question is what should inform what we support in schematics.

@jelbourn
Copy link
Member

Do we want or intend to be more restrictive about default export in the Angular style guide?

I think this is mostly a CLI question (aka a Doug question) since it most heavily impacts the build and bundling aspect of Angular rather than any framework concepts. My inclination is to not introduce any restrictions without a strong technical reason to do so.

Do we want to encourage or recommend default export over named exports for routing use cases?

I don't know about encourage. I don't love 3P guidance diverging from Google internal guidance (over which we have limited sway). My inclination is to take no formal stance from a style guide perspective.

Does this fit in ng generate component? Does it fit in ng generate route instead?

I think adding an option is reasonable. I don't love the idea of ng g route, though, since it muddies the water as to types of Angular construct (i.e., I don't want people writing blog posts about "Route components" vs "component components")

@angular-robot angular-robot bot added the feature: votes required Feature request which is currently still in the voting phase label Apr 15, 2023
@angular-robot
Copy link
Contributor

angular-robot bot commented Apr 15, 2023

This feature request is now candidate for our backlog! In the next phase, the community has 60 days to upvote. If the request receives more than 20 upvotes, we'll move it to our consideration list.

You can find more details about the feature request process in our documentation.

@angular-robot angular-robot bot added feature: under consideration Feature request for which voting has completed and the request is now under consideration and removed feature: votes required Feature request which is currently still in the voting phase labels Apr 18, 2023
@aparzi
Copy link
Contributor

aparzi commented Jan 9, 2024

Hi @alan-agius4,
is anyone working on this issue? Otherwise I could do it myself

@dgp1130
Copy link
Collaborator

dgp1130 commented Aug 21, 2024

We had some more discussion today based on recent community interest in this.

We're ok with adding a form of --export-default. I don't think any of us are super excited about the option given that most users are unlikely to discover this or remember to use it and instead just manually add export when needed. We generally like to avoid a large number of small conditionals like --display-block. However there's enough upvotes on this issue to justify at least trying it out. I'd prefer --export-default over --default-export given that aligns with the ordering you'll use when writing export default in actual JavaScript, so it feels easier to remember to me.

Longer term, it might be worth adding some kind of ng generate route which creates a component (with --export-default) and also adds it to the route list. Unfortunately this is tricky to get right as we'd need to know whether to use loadComponent or loadChildren and find the routes.ts file, which could be renamed. That's probably out of scope for now, but it's a potential future feature to consider.

If anyone would like to contribute an --export-default option, it's probably a good first contribution as it's fairly well-scoped to schematics. You'd need to add a new exportDefault option here and update the template accordingly

@dgp1130 dgp1130 added good first issue and removed needs: discussion On the agenda for team meeting to determine next steps labels Aug 21, 2024
@brandonroberts
Copy link
Contributor Author

Awesome. Thanks for the update and consideration!

@aparzi
Copy link
Contributor

aparzi commented Aug 22, 2024

Hi @dgp1130,
I have implemented --export-default, can you help me understand how i can test the command locally?

Thanks.

@dgp1130
Copy link
Collaborator

dgp1130 commented Aug 22, 2024

@aparzi, you should definitely add some tests to https://github.com/angular/angular-cli/blob/e40384e637bc6f92c28f4e572f986ca902938ba2/packages/schematics/angular/component/index_spec.ts, which should provide some reasonable confidence.

If you want to test it out in your own application, yarn build --local should build the packages into the dist/ directory. You can then install the @schematics/angular tarball into any Angular app test out ng generate component --export-default directly.

More details: https://github.com/angular/angular-cli/blob/main/docs/DEVELOPER.md

@aparzi
Copy link
Contributor

aparzi commented Aug 22, 2024

@dgp1130 ok thanks.
I open a merge request. I have made the implementation and written some tests

aparzi added a commit to aparzi/angular-cli that referenced this issue Aug 22, 2024
added option for export class in default mode

Closes angular#25023
aparzi added a commit to aparzi/angular-cli that referenced this issue Aug 23, 2024
Introduces option `--export-default` to control whether the generated component uses a default export instead of a named export.

Closes: angular#25023
aparzi added a commit to aparzi/angular-cli that referenced this issue Aug 23, 2024
Introduces option `--export-default` to control whether the generated component uses a default export instead of a named export.

Closes: angular#25023
aparzi added a commit to aparzi/angular-cli that referenced this issue Aug 23, 2024
Introduces option `--export-default` to control whether the generated component uses a default export instead of a named export.

Closes: angular#25023
aparzi added a commit to aparzi/angular-cli that referenced this issue Aug 23, 2024
Introduces option `--export-default` to control whether the generated component uses a default export instead of a named export.

Closes: angular#25023
@aparzi
Copy link
Contributor

aparzi commented Aug 24, 2024

Hi @alan-agius4,
should this part of the documentation be updated with the new option?

Thanks

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Aug 24, 2024

The option will automatically be updated.

See: angular/angular#57508

@aparzi
Copy link
Contributor

aparzi commented Aug 24, 2024

Awesome

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: @schematics/angular feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants