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

🚨 Lint against extraneous dependencies #670

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

wwilsman
Copy link
Contributor

What is this?

In all circumstances where it matters in this repo, the extraneous dependencies are guaranteed to be present and resolve to their expected versions. However, to better support Yarn 2+ and any other potential node loaders that might block on this, this eslint rule will help ensure that packages only import from their specific dependencies.

The rule is disabled in tests where it usually complains about other packages in the workspace which are always available during development.

Where utils were being transitively imported through @percy/core dependencies, are now imported through the @percy/core package itself (with help of a recent change re-exporting client's request util).

In all circumstances where it matters in this repo, the extraneous dependencies are guaranteed to be
present and resolve to their expected versions. However, to better support Yarn 2+ and any other
potential node loaders that might block on this, this eslint rule will help ensure that packages
only import from their specific dependencies.
@wwilsman wwilsman added the 🧹 maintenance General maintenance label Dec 14, 2021
let url = percy.address.replace('http', 'ws');

if (process.env.__PERCY_BROWSERIFIED__) {
return new window.WebSocket(url);
} else {
let socket = new (require('ws'))(url);
/* eslint-disable-next-line import/no-extraneous-dependencies */
let { default: WebSocket } = await import('ws');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While here, I also switched this to a dynamic import rather than a commonjs require in preparation for a full ES module adaptation in the future (dynamic imports will work in the current commonjs environment)

@wwilsman wwilsman enabled auto-merge (squash) December 14, 2021 00:26
Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁

@csi-lk
Copy link

csi-lk commented Dec 15, 2021

Thanks for the work on this @wwilsman

@hellodave76
Copy link

Thanks from me as well @wwilsman 👍

Any idea when this will be released? Or know a workaround until it is?
I'm having the same issue as #635

@wwilsman
Copy link
Contributor Author

@hellodave76 If you're on Yarn 2+, you can use a .yarnrc file with a packageExtensions field: https://yarnpkg.com/configuration/yarnrc#packageExtensions

@hellodave76
Copy link

thanks @wwilsman that worked 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 maintenance General maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants