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.

Sign up for GitHub

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

Jump to bottom

feat(transformer): add remove-vue-import transformer #7

Merged
merged 1 commit into from Oct 1, 2020
Merged

feat(transformer): add remove-vue-import transformer #7

merged 1 commit into from Oct 1, 2020

Conversation

ahnpnl
Copy link
Collaborator

@ahnpnl ahnpnl commented Sep 20, 2020

Closes #4

@ahnpnl ahnpnl force-pushed the remove-vue-import-transformer branch from 8c95059 to 4a4f579 Compare September 20, 2020 13:51
@ahnpnl ahnpnl marked this pull request as draft September 20, 2020 13:52
@ahnpnl ahnpnl force-pushed the remove-vue-import-transformer branch from 4a4f579 to 888322f Compare September 20, 2020 14:41
@ahnpnl ahnpnl marked this pull request as ready for review September 20, 2020 14:41
@ahnpnl
Copy link
Collaborator Author

ahnpnl commented Sep 20, 2020

should be ready now :)

@ahnpnl ahnpnl changed the title feat(transformer): add remove-vue-import transformer feat(transformer): add no-duplicated-global-import transformer Sep 20, 2020
@ahnpnl ahnpnl force-pushed the remove-vue-import-transformer branch from 888322f to 7b0868c Compare September 20, 2020 14:47
@ahnpnl ahnpnl changed the title feat(transformer): add no-duplicated-global-import transformer feat(transformer): add no-duplicated-vue-import transformer Sep 20, 2020
@ahnpnl ahnpnl force-pushed the remove-vue-import-transformer branch 2 times, most recently from 8bb5769 to 2775ee1 Compare September 20, 2020 18:20
ahnpnl
ahnpnl commented Sep 20, 2020
View reviewed changes
tests/__snapshots__/no-duplicated-vue-import.spec.ts.snap Outdated
var vue1 = require(\\"vue\\");
var vue2 = require('vue');
var vue3 = require(\\"vue\\");
exports.default = vue_2.defineComponent({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm actually this vue_2 should be vue_1 (which is global import

Copy link
Collaborator Author

@ahnpnl ahnpnl Sep 20, 2020 •
edited

Choose a reason for hiding this comment

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

I am thinking about removing all imports and reference all usages to global Vue import vue_1

Copy link
Owner

Choose a reason for hiding this comment

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

Seems like ideally we should only have one import.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok I will try to look in that way

lmiller1990
lmiller1990 reviewed Sep 21, 2020
View reviewed changes
Copy link
Owner

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

This is a very useful PR for me to learn how to use the TS compiler AST

tests/__snapshots__/no-duplicated-vue-import.spec.ts.snap Outdated
var vue1 = require(\\"vue\\");
var vue2 = require('vue');
var vue3 = require(\\"vue\\");
exports.default = vue_2.defineComponent({
Copy link
Owner

Choose a reason for hiding this comment

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

Seems like ideally we should only have one import.

src/transformers/no-duplicated-vue-import.ts Outdated
* }
* }
* }
*/
Copy link
Owner

Choose a reason for hiding this comment

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

wow, amazing. will the user need to duplicate this jest.config, or can we merge it in to their own jest config somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it depends on how often users use es6+ in tsconfig.spec.json .

If it is often the case, you can make a preset file and let users reference to that preset. The preset will contain this config.

If it is not often the case, users can opt-in/out this transformer.

Copy link
Owner

Choose a reason for hiding this comment

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

great. most of the users are not writing their own config anyway, but using a template. this will be good.

@ahnpnl ahnpnl marked this pull request as draft September 21, 2020 09:48
@ahnpnl ahnpnl force-pushed the remove-vue-import-transformer branch 5 times, most recently from 6be0b20 to 0c7875d Compare September 29, 2020 15:09
@ahnpnl ahnpnl marked this pull request as ready for review September 29, 2020 15:09
@ahnpnl ahnpnl changed the title feat(transformer): add no-duplicated-vue-import transformer feat(transformer): add remove-vue-import transformer Sep 29, 2020
@ahnpnl ahnpnl force-pushed the remove-vue-import-transformer branch from 0c7875d to b76ab1e Compare September 29, 2020 16:26
@ahnpnl
Copy link
Collaborator Author

ahnpnl commented Sep 29, 2020

Caveats this transformer doesn't take care of these 2 syntaxes:

import Vue from 'vue'

and

import vue = require('vue')

@lmiller1990
Copy link
Owner

Sorry - forgot about this. I am not sure import Vue from 'vue' is especially common in SFCs - should be okay.

I will try this out and merge this today or tomorrow. Thanks @ahnpnl!

@lmiller1990 lmiller1990 merged commit 13b5491 into master Oct 1, 2020
@lmiller1990 lmiller1990 deleted the remove-vue-import-transformer branch October 1, 2020 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmiller1990 lmiller1990 Awaiting requested review from lmiller1990

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle tsconfig target higher than es5
2 participants
@ahnpnl @lmiller1990