-
Notifications
You must be signed in to change notification settings - Fork 414
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
fix(deps): drop a few unnecessary dependencies (#643) #654
Conversation
return ( | ||
literal: TemplateStringsArray, | ||
...placeholders: string[] | ||
): [string, Map<string, string[]>] => { | ||
const code = dedent(literal, ...placeholders); | ||
const [shaken, deps] = shake( | ||
babel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the benefit of injecting Babel here? I don't think we'll inject various versions of Babel and it stays as dependency, but peer. I think we should be fine by leaving these functions as they were – dependent on @babel/core
. This would shrink the diff quite a bit :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm… We don't touch the original AST in evaluators, so it's probably safe to use imported babel here. I'll take a close look.
docs/HOW_IT_WORKS.md
Outdated
@@ -174,6 +174,7 @@ Sometimes it can be useful to implement your own strategy (it can be just a mock | |||
|
|||
```typescript | |||
type Evaluator = ( | |||
babel: BabelCore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this change imho, as stated somewhere earlier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is the last thing to revert now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool move! Don't forget about mentioning @babel/core
as peer dependency in the readme
Why do we need type-only imports? |
@wereHamster mostly because of linter: eslint doesn't complain about type-only imports if dependencies aren't specified in package.json. Another reason is improved readability, that helps to manage dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
docs/HOW_IT_WORKS.md
Outdated
@@ -174,6 +174,7 @@ Sometimes it can be useful to implement your own strategy (it can be just a mock | |||
|
|||
```typescript | |||
type Evaluator = ( | |||
babel: BabelCore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is the last thing to revert now :)
Motivation
We have some dependencies which we don't really use or can avoid using: webpack, @babel/core, etc.
Another problem here, that Linaria babel plugin uses an external version of babel instead of a version which launches the plugin, that can probably cause some tricky errors.
Summary
Despite the fact that PR touches a lot of files, the most changes are just
type
annotations for imports and linted files (update of eslint was required for supportingtype
). The real change here is passingbabel
through call stacks in the plugin, but it doesn't affect the logic.There are also three minor fixes:
expressionsToEvaluate
andoriginalLazyExpressions
;Results:
Test plan
Since the logic wasn't affected, all existed tests are still working.