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

Angular: Fix angular2-template-loader / raw-loader version conflicts #8269

Merged
merged 6 commits into from
Oct 11, 2019

Conversation

kroeder
Copy link
Member

@kroeder kroeder commented Oct 2, 2019

Issue: #7877

What I did

As suggested in #7877 (comment) I fixed this issue by modifying the code of angular2-template-loader

Though, instead of using patch-package I just copied the loader function from the angular2-template-loader repository. This package hasn't received any updates in the past 3 years and I contacted the owner to talk about the future of this package.

Also, copying the loader also means we don't have to install another dependency that also needs to be executed as a postinstall script.

How to test

  • Checkout this branch
  • Execute the prepare script in app/angular
  • Start the angular example in examples/angular-cli
  • Go to the AppComponent story
    • If it works you see the initial angular welcome page
    • It does not work if you see an error like this
@storybook/angular: Expected 'styles' to be an array of strings

What else

We should make sure this works in Angular 7 and 8 before merging it

@vercel
Copy link

vercel bot commented Oct 2, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/2j2w5a1at
🌍 Preview: https://monorepo-git-fix-issue7877-webpack-styles-loader.storybook.now.sh

@kroeder
Copy link
Member Author

kroeder commented Oct 2, 2019

Working example for Angular 8: https://monorepo-git-fix-issue7877-webpack-styles-loader.storybook.now.sh/angular-cli/?path=/story/app-component--component-with-separate-template

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM! 😭

@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Oct 2, 2019
@kroeder kroeder changed the title WIP: fix angular2-template-loader / raw-loader version conflicts fix angular2-template-loader / raw-loader version conflicts Oct 2, 2019
@kroeder
Copy link
Member Author

kroeder commented Oct 2, 2019

Added DO NOT MERGE. I want to improve this weird require string / length check

@kroeder
Copy link
Member Author

kroeder commented Oct 9, 2019

I found this test file in the angular internals https://github.com/angular/angular-cli/blob/0aae1476218c95a38527877e15ae55ba68913400/packages/ngtools/webpack/src/transformers/replace_resources_spec.ts and thought we can maybe just use what angular uses

This worked locally and I even could get rid of two webpack plugins but this is all still just WIP. I need to figure out a couple of things

  • Is this backwards compatible, if not, can I make it backwards compatible?
    • Works with angular 7 & 8 and raw-loader 1.0.0 and > 1.0.0
    • The current fix does not work in my companies repo due to some compiler issues AppComponent(?, ?) error. Often refers to dependency injection issues. It's weird because it works with a fresh project that uses AppComponent as a story component. I need to figure this out first
  • Can I really remove the ForkTsChecker Plugin that decreased the angular build time a lot?
    • I'm not sure. Start-time on my machine in a 60 story project was about 40~ seconds without and 50~ with. I'd say let's keep it and don't change a running system.
  • Can I really remove? I can't.
      new ContextReplacementPlugin(
        /@angular(\\|\/)core(\\|\/)(fesm5|bundles)/,
        path.resolve(__dirname, '..')
      ),

Benefits of this approach

  • Getting rid of an unmaintained package (angular2-template-loader) by writing a small loader that uses angular internals
  • Maybe being more stable that way, depending on if this is backwards compatible

@kroeder kroeder force-pushed the fix-issue7877-webpack-styles-loader branch from 9d5439d to 19e8024 Compare October 11, 2019 14:58
@vercel vercel bot temporarily deployed to staging October 11, 2019 14:58 Inactive
@kroeder
Copy link
Member Author

kroeder commented Oct 11, 2019

I reverted my new approach. It's unsafe and I couldn't make it compatible for all my test projects. This implementation is a safe fix and solves peoples problems

@shilman shilman changed the title fix angular2-template-loader / raw-loader version conflicts Angular: Fix angular2-template-loader / raw-loader version conflicts Oct 11, 2019
@shilman shilman merged commit 2dd1dac into next Oct 11, 2019
@shilman shilman deleted the fix-issue7877-webpack-styles-loader branch October 11, 2019 20:08
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Oct 14, 2019
shilman added a commit that referenced this pull request Oct 14, 2019
Angular: Fix angular2-template-loader / raw-loader version conflicts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
angular bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants