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

feat: add support for building with vue cli #60

Merged
merged 5 commits into from
Jan 30, 2021
Merged

Conversation

nklayman
Copy link
Member

If vue-cli-plugin-tauri is installed, it will run yarn tauri:build instead of yarn tauri build.

@nklayman
Copy link
Member Author

Something is broken, seems like it's the getDependency function, but I'm really not sure. Here is the failing run. I can't figure out what is wrong, maybe another pair of eyes can catch something.

@nklayman
Copy link
Member Author

Okay now that I've actually built the project it works. @jbolda can you review and merge please?

@nklayman nklayman marked this pull request as ready for review January 29, 2021 23:05
@@ -145,8 +146,11 @@ async function buildProject(
}

const args = debug ? ['--debug'] : []
const buildCommand = hasDependency('vue-cli-plugin-tauri', root)
? (usesYarn(root) ? 'yarn' : 'npm run') + ' tauri:build'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
? (usesYarn(root) ? 'yarn' : 'npm run') + ' tauri:build'
? (npmScript || (usesYarn(root) ? 'yarn' : 'npm run') + ' tauri:build')

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah that's not a bad idea. However, the npm script is expected to be used like this: yarn/npm run ${npmScript} build. Is it a bad idea to make it function slightly differently in the case of the vue cli plugin? We would have to update the docs as well. At the very least it should probably still add yarn/npm run automatically.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could leave it the way you wrote since this action is mostly for simple apps

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I doubt people will change the script name, if someone needs to and opens and issue we can just add it then. Ready to merge IMO 👍.

Copy link
Member

@jbolda jbolda left a comment

Choose a reason for hiding this comment

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

Seems to fit with the existing precedent you have with vue. We need to add a change file, then it's good to go.

@nklayman
Copy link
Member Author

Okay I added one, lmk if I did it right.

@nklayman
Copy link
Member Author

Alright there we go, fixed now.

@lucasfernog lucasfernog merged commit f043343 into dev Jan 30, 2021
@lucasfernog lucasfernog deleted the vue-cli-support branch January 30, 2021 19:22
@github-actions github-actions bot mentioned this pull request Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants