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: add nativescript-vue-template-compiler support (#354) #355

Merged
merged 1 commit into from
Nov 8, 2019
Merged

feat: add nativescript-vue-template-compiler support (#354) #355

merged 1 commit into from
Nov 8, 2019

Conversation

cainrus
Copy link
Contributor

@cainrus cainrus commented Nov 2, 2019

PR for #354

  • feat: add nativescript-vue-template-compiler support

  • test: add vue related unit and integrations tests

  • docs: add information about nativescript-vue-template-compiler support

@cainrus
Copy link
Contributor Author

cainrus commented Nov 2, 2019 •
edited

I don't have much experience with yarn.
Do I need to do something with yarn lock file to pass the ci tests?

@johnnyreilly
Copy link
Member

I believe you've changed the package.json but you didn't do so with yarn and so the yarn.lock file wasn't regenerated. You're going to need to use yarn to regenerate the lock file. Simply executing yarn where you would otherwise npm install should do it

@cainrus cainrus force-pushed the feat/support-nativescript-vue-template-compiler branch from 543dd12 to 6ece55e Compare November 2, 2019 09:43
@cainrus
Copy link
Contributor Author

cainrus commented Nov 2, 2019

@johnnyreilly
Can you take a look for PR?

@johnnyreilly
Copy link
Member

In holiday right now - will check soon. Thanks!

johnnyreilly
johnnyreilly requested changes Nov 2, 2019
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/IncrementalChecker.ts Show resolved Hide resolved
@johnnyreilly
Copy link
Member

Looks pretty good to me; though I'm mindful I'm not too knowledgeable about Vue. cc @yyx990803 in case there's any views on this? (I think this plugin is used in the Vue CLI so thought I'd give a heads up)

@cainrus cainrus force-pushed the feat/support-nativescript-vue-template-compiler branch from 6ece55e to aafe8cd Compare November 2, 2019 21:31
johnnyreilly
johnnyreilly previously approved these changes Nov 3, 2019
View reviewed changes
Copy link
Member

@johnnyreilly johnnyreilly left a comment

Choose a reason for hiding this comment

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

I'm happy with this. If @piotr-oles / @phryneas are cool with this I'm fine with this shipping.

It would be great to get some Vue user opinion in the mix too

@phryneas
Copy link
Contributor

phryneas commented Nov 3, 2019

Code-wise, this looks good to me. But I'm not 100% happy with the integration tests, as this doesn't test if the actual compilation happens with nativescript-vue-template-compiler at all, if I'm reading it right.

Maybe duplicate the whole test suite with describe.each like in line 25 and mock the respective "other" compiler into one that throws an Error when called to make sure it's the right one being used? (Or spy on the "right one" to make sure it's being used - or a combination of both.)

piotr-oles
piotr-oles requested changes Nov 3, 2019
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
test/integration/vue.spec.ts Outdated Show resolved Hide resolved
test/integration/vue.spec.ts Outdated Show resolved Hide resolved
@cainrus cainrus force-pushed the feat/support-nativescript-vue-template-compiler branch from aafe8cd to 443c74d Compare November 5, 2019 00:55
@cainrus
Copy link
Contributor Author

cainrus commented Nov 5, 2019 •
edited

Maybe duplicate the whole test suite with describe.each

Good idea. in progress right now

@cainrus cainrus force-pushed the feat/support-nativescript-vue-template-compiler branch from 443c74d to 818196d Compare November 6, 2019 00:28
@cainrus
Copy link
Contributor Author

cainrus commented Nov 6, 2019

@johnnyreilly, @phryneas, @piotr-oles
I think i'm done with review fixes. Do you want to review it again?

@cainrus cainrus force-pushed the feat/support-nativescript-vue-template-compiler branch from 818196d to 5aa348e Compare November 6, 2019 00:48
phryneas
phryneas suggested changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

Just some things I saw at first glance - have to give this a better review later, or tomorrow.

test/integration/vue.spec.ts Outdated Show resolved Hide resolved
test/integration/vue.spec.ts Outdated Show resolved Hide resolved
@cainrus
feat: add nativescript-vue-template-compiler support (TypeStrong#354)
818aa58 
* feat: add nativescript-vue-template-compiler support

* test: add vue related unit and integrations tests

* docs: add information about nativescript-vue-template-compiler support
@cainrus cainrus force-pushed the feat/support-nativescript-vue-template-compiler branch from 5aa348e to 818aa58 Compare November 7, 2019 00:35
@cainrus cainrus requested a review from phryneas November 7, 2019 00:40
phryneas
phryneas approved these changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

looks good to me

@piotr-oles piotr-oles merged commit 032475e into TypeStrong:master Nov 8, 2019
@piotr-oles
Copy link
Collaborator

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@phryneas phryneas mentioned this pull request Nov 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phryneas phryneas phryneas approved these changes

@piotr-oles piotr-oles piotr-oles approved these changes

@johnnyreilly johnnyreilly johnnyreilly left review comments

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@cainrus @johnnyreilly @phryneas @piotr-oles