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

Using webpack aliases in simorgh #3542

Closed
wants to merge 10 commits into from
Closed

Conversation

oluoluoxenfree
Copy link
Contributor

@oluoluoxenfree oluoluoxenfree commented Sep 3, 2019

Resolves #2567

Overall change:

Uses webpack directory aliasing previously set up in #3318

Code changes:

  • A bullet point list of key code changes that have been made.
  • When describing code changes, try to communicate how and why you implemented something a specific way, not just what has changed.

  • I have assigned myself to this PR and the corresponding issues
  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@oluoluoxenfree oluoluoxenfree self-assigned this Sep 4, 2019
@oluoluoxenfree oluoluoxenfree added this to PR in Progress in Simorgh via automation Sep 4, 2019
@oluoluoxenfree oluoluoxenfree moved this from PR in Progress to Code review in Simorgh Sep 4, 2019
Copy link
Contributor

@benjaminhobbs benjaminhobbs left a comment

Choose a reason for hiding this comment

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

Found those in storybook and cypress need reverting, they don't share the same webpack setup so cannot use these aliases at all

Simorgh automation moved this from Code review to PR in Progress Sep 4, 2019
@benjaminhobbs
Copy link
Contributor

benjaminhobbs commented Sep 4, 2019

FYI @hindsc52 @dr3

Ok so problems in order:

  1. At first glance this looks pretty good. But the build passes when it absolutely shouldn't! Storybook won't run, the only e2es that still run are those missed in the PR (cypress/integration/application/index.js)
  2. Some files are missed as proved by some e2es still working in point 1
  3. Some files changed use app when they could directly use lib. I fixed a couple and later realised I missed some more, but this doesn't really matter because...
  4. This approach cannot work at all. The webpack config is for our app, the webpack config is not shared in cypress, 3rdPartyCypress or storybook - so every single alias reference in there fails - without erroring the build. This is the cause of point 1. We cannot even remove change to storybook because the story files themselves have aliases that it cannot understand. If we reverted these back them I'm not certain the components themselves could render because it'll still try to render those components in storybook with the unresolvable aliases. So we'd then have to not use aliases in components/containers or we'd have to extend the webpack config to work for storybook. But is another setup task to be done before this.

Is all this worth the aliases? I thought this'd be easy when I wrote the issue. I'm tempted to revert the addition of aliases and not do this at all.

Horrifying that such an innocuous seeming PR can cause a confusing thing as a passing build with nearly everything outside the main app failing.

PS how can we avoid all tests not running and the build still passing in future PRs? That seems much more important to fix that anything we're trying to do here.

PPS @oluoluoxenfree don't let this get you down, we all missed the dragons herein when we approved the preparatory PR - maybe Chris or Drew have a quickfix, but I can't think of one.

@oluoluoxenfree
Copy link
Contributor Author

@benjaminhobbs can we not do something like this?

storybookjs/storybook#3339

@thekp thekp mentioned this pull request Sep 11, 2019
3 tasks
@thekp
Copy link
Contributor

thekp commented Sep 12, 2019

I am closing this PR, to continue it here : #3707

@thekp thekp closed this Sep 12, 2019
Simorgh automation moved this from PR in Progress to Done Sep 12, 2019
@thekp thekp deleted the use-webpack-aliases branch September 12, 2019 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add webpack directory aliasing
4 participants