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

Restructure repo to report on missing dependencies #1026

Closed
wants to merge 10 commits into from

Conversation

cartogram
Copy link
Contributor

@cartogram cartogram commented Sep 22, 2019

Description

Fixes #1001

The changes in this PR allow us to turn on the eslint rule no-extraneous-dependencies. This is a big change so I'm breaking down the three major things happening here:

  1. I moved all of the packages to packages/@shopify to allow the eslint rule to recognize the packages in this repository properly. More context in this issue Prevent missing @shopify/* packages from going out to releases #1001

  2. Adjusted our .eslintrc to automatically populate the overrides for each package in this repo to only look locally in that package for existing dependencies.

  3. With the above 2 changes, it now successfully reporting errors and the final changes are me retro-actively applying the missing dependencies for the ~200 times we broke this rule, but got lucky in that it didn't break anything.

Additional things

  • Point 3 is work in progress, I want to get a general approval before going through that additional effort.
  • In the config for the rule I chose to devDependencies: true, optionalDependencies: false, peerDependencies: true, as it seemed like the best way to accommodate the missing react/react-dom dependencies across this repo.
  • This PR seems a little over-the-top, but I think organizing the repo in this way would have been good idea anyways. Mat brought up the point of migrating to a quilt framework more easily which I agree with.
  • Anything repercussions of this change that I am missing? For example, additional libraries being in conflict when packages are installed in other projects?

Matt Seccafien added 10 commits September 17, 2019 11:23
line 65 of importType.js from eslint-plugin-import should be `return -1 < path.indexOf((0, _path.join)(folder, packageName)) || -1 < path.indexOf((0, _path.join)(folder, name.split(‘/‘)[1]))`
Copy link
Member

@GoodForOneFare GoodForOneFare left a comment

Choose a reason for hiding this comment

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

I'd prefer it if this PR was just step 1. It's unlikely that I'll accurately review this once 200+ dependency fixes are in.

@GoodForOneFare
Copy link
Member

^ That aside, great job on figuring all of this out 👍 It's well on the way to solving a significant pain point for us and our consumers 🎉

@cartogram
Copy link
Contributor Author

Closing this PR. @lemonmade has a different solution in the works that doesn't require the massive restructure.

@cartogram cartogram closed this Sep 26, 2019
@cartogram cartogram deleted the no-extraneous-packages branch November 8, 2019 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent missing @shopify/* packages from going out to releases
2 participants