Skip to content

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

Merged
piotr-oles merged 1 commit intoTypeStrong:masterfrom
cainrus:feat/support-nativescript-vue-template-compiler
Nov 8, 2019
Merged

feat: add nativescript-vue-template-compiler support (#354)#355
piotr-oles merged 1 commit intoTypeStrong:masterfrom
cainrus:feat/support-nativescript-vue-template-compiler

Conversation

@cainrus
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

cainrus commented Nov 2, 2019 •
edited
Loading

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
Copy Markdown
Member

johnnyreilly commented Nov 2, 2019

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
Copy Markdown
Contributor Author

cainrus commented Nov 2, 2019

@johnnyreilly
Can you take a look for PR?

@johnnyreilly
Copy link
Copy Markdown
Member

johnnyreilly commented Nov 2, 2019

In holiday right now - will check soon. Thanks!

johnnyreilly
johnnyreilly requested changes Nov 2, 2019
View reviewed changes
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread src/IncrementalChecker.ts
@johnnyreilly
Copy link
Copy Markdown
Member

johnnyreilly commented Nov 2, 2019

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
Copy Markdown
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
Copy Markdown
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
Comment thread src/index.ts Outdated
Comment thread test/integration/vue.spec.ts Outdated
Comment thread test/integration/vue.spec.ts Outdated
@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
Copy Markdown
Contributor Author

cainrus commented Nov 5, 2019 •
edited
Loading

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
Copy Markdown
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 requested a review from piotr-oles November 6, 2019 00:40
@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
Copy Markdown
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.

Comment thread test/integration/vue.spec.ts Outdated
Comment thread test/integration/vue.spec.ts Outdated
@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
Copy Markdown
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
Copy Markdown
Collaborator

piotr-oles commented Nov 8, 2019

🎉 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

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

@johnnyreilly johnnyreilly johnnyreilly left review comments

+1 more reviewer

@phryneas phryneas phryneas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cainrus @johnnyreilly @phryneas @piotr-oles