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

bug(mdc-migration): Migrate components selectively is bugged #26426

Closed
1 task
llorenspujol opened this issue Jan 13, 2023 · 7 comments · Fixed by #26427
Closed
1 task

bug(mdc-migration): Migrate components selectively is bugged #26426

llorenspujol opened this issue Jan 13, 2023 · 7 comments · Fixed by #26427
Labels
needs triage This issue needs to be triaged by the team

Comments

@llorenspujol
Copy link
Contributor

llorenspujol commented Jan 13, 2023

Is this a regression?

  • Yes, this behavior used to work in the previous version

The previous version in which this bug was not present was

No response

Description

When Migrating to MDC-based Angular Material Components, and doing it component by component, it should only migrate the selected components. Right now other components that are not selected are also migrated. Seems that it only happens on imports.

Take for example this ngModule code:

      import {NgModule} from '@angular/core';
      import {MatLegacyButtonModule} from '@angular/material/legacy-button';
      import {MatLegacyCheckboxModule} from '@angular/material/legacy-checkbox';

      @NgModule({imports: [MatLegacyButtonModule, MatLegacyCheckboxModule]})
      export class AppModule {}

Right now, migrating this file selecting only the "button" migrates also the Checkbox. This should not happen.

Reproduction

Steps to reproduce:

Execute the migration for only "button" on this ngModule code:

      import {NgModule} from '@angular/core';
      import {MatLegacyButtonModule} from '@angular/material/legacy-button';
      import {MatLegacyCheckboxModule} from '@angular/material/legacy-checkbox';

      @NgModule({imports: [MatLegacyButtonModule, MatLegacyCheckboxModule]})
      export class AppModule {}

Expected Behavior

Should migrate only selected components

Actual Behavior

Selectively migrating components is buggy and in some cases, like imports, other components are also migrated

Environment

  • Angular: 15.1.0
  • CDK/Material: 15.1.0
  • Browser(s): -
  • Operating System (e.g. Windows, macOS, Ubuntu): Ubuntu
@llorenspujol llorenspujol added the needs triage This issue needs to be triaged by the team label Jan 13, 2023
llorenspujol added a commit to llorenspujol/components that referenced this issue Jan 13, 2023
Fixes mdc-migration to migrate only the selected components.

Fixes angular#26426
@JaxonWright
Copy link

I can confirm that this happens. It has resulted in me just having to do it all in one go and deal with the pain of making sure everything looks good.

@llorenspujol
Copy link
Contributor Author

llorenspujol commented Jan 17, 2023

For anyone in this thread, there is one workaround that I have been using. Basically is 'patching' the mdc-migration directly on the '/node_modules' forlder, there is a quite easy way to do it.

1- Go to the file: /node_modules/@angular/material/schematics/ng-generate/mdc-migration/index_bundled.js.
2- Search for the variable LEGACY_MODULES.
3- Comment ALL the components that you don't want to migrate! For example, just migrating the button would result in:

var LEGACY_MODULES = new Set([
  // "legacy-autocomplete",
  // "legacy-autocomplete/testing",
  "legacy-button",
  "legacy-button/testing",
  // "legacy-card",
  // "legacy-card/testing",
  // ........
]);

This is just a workaround solution that you could use while this bug is not fixed.
Having said that I hope the angular team can revise it and accept this MR to fix it. This bug would actually prevent many users to upgrading to MDC.

@ectuser
Copy link

ectuser commented Jan 17, 2023

I confirm that have experienced the same bug

@bampakoa
Copy link
Contributor

In the case of using a custom theme file, I have noticed that it includes theme styles for all MDC components:

// Include theme styles for core and each component used in your app.
// Alternatively, you can import and @include the theme mixins for each component
// that you are using.
/* TODO(mdc-migration): Remove all-legacy-component-themes once all legacy components are migrated*/
@include mat.all-legacy-component-themes($my-theme);
@include mat.all-component-themes($my-theme); 

I wonder if it needs to be changed so that it includes only those styles that are used by the migrating components such as:

@include mat.all-legacy-component-themes($my-theme);
@include mat.chip-themes($my-theme); 
@include mat.button-themes($my-theme); 

@JaxonWright
Copy link

JaxonWright commented Jan 17, 2023

@bampakoa that seems more like a nice-to-have that is related to this issue. Nothing breaks when you include both all of the legacy and all of time MDC themes. That is because there is no cross-over between the two.

However, selectively importing the styles of only the migrated components would save on bundle size if it isn't tree-shakable (I don't know if this is or not).

mmalerba pushed a commit to llorenspujol/components that referenced this issue Jan 31, 2023
Fixes mdc-migration to migrate only the selected components.

Fixes angular#26426
@michaelfaith
Copy link

Just encountered this myself. Thanks for jumping on it.

llorenspujol added a commit to llorenspujol/components that referenced this issue Feb 2, 2023
Fixes mdc-migration to migrate only the selected components.

Fixes angular#26426
llorenspujol added a commit to llorenspujol/components that referenced this issue Feb 2, 2023
Fixes mdc-migration to migrate only the selected components.

Fixes angular#26426
llorenspujol added a commit to llorenspujol/components that referenced this issue Feb 3, 2023
Fixes mdc-migration to migrate only the selected components.

Fixes angular#26426
@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 Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs triage This issue needs to be triaged by the team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants