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

chore(scripts): add prepare script #1812

Merged
merged 1 commit into from Oct 14, 2022
Merged

chore(scripts): add prepare script #1812

merged 1 commit into from Oct 14, 2022

Conversation

xanf
Copy link
Collaborator

@xanf xanf commented Oct 12, 2022

This is extremely simple, yet useful (in my opinion) change.

NOTE: If a package being installed through git contains a prepare script, its dependencies and devDependencies will be installed, and the prepare script will be run, before the package is packaged and installed.

Since npm v5 npm executes prepare script when dependency is installed from git. By including prepare script it will allow our users use @vue/test-utils from main (or any other branch) even if release was not published to npm yet

However, this has a downside - after doing install locally (without include @vue/test-utils as dependency) it will be also executed. However, I feel here benefits outweight the downside

WDYT?

@netlify
Copy link

netlify bot commented Oct 12, 2022

Deploy Preview for vue-test-utils-docs ready!

Name Link
🔨 Latest commit d79e46e
🔍 Latest deploy log https://app.netlify.com/sites/vue-test-utils-docs/deploys/6347212295c0ea0008331b67
😎 Deploy Preview https://deploy-preview-1812--vue-test-utils-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@xanf xanf changed the title chore(scripts): add prebuild script chore(scripts): add prepare script Oct 12, 2022
@@ -76,6 +76,7 @@
"test:build": "vitest --mode test-build",
"tsd": "tsc -p test-dts/tsconfig.tsd.json",
"build": "rollup -c rollup.config.js",
"prepare": "rollup -c rollup.config.js",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're duplicating build script instead of pnpm run build - we do not want to rely on pnpm, and having build invoked as pnpm run prepare feels very wrong to me

@cexbrayat
Copy link
Member

I don't have a strong opinion about this one, I don't think I ever used prepare.

@lmiller1990 @freakzlike WDYT?

@xanf
Copy link
Collaborator Author

xanf commented Oct 13, 2022

I don't have a strong opinion about this one, I don't think I ever used prepare.

The great thing is it used automatically and our users will be able to use main branch if needed

@freakzlike
Copy link
Collaborator

I think this is good. Not sure if that will affect the release process

@xanf
Copy link
Collaborator Author

xanf commented Oct 13, 2022

@freakzlike not at all. We have extra build triggered inside pipeline (you can see even in this PR that prepare was called during npm install) but it doesn't affect released package at all

@cexbrayat
Copy link
Member

Let's go ahead and try it then

@cexbrayat cexbrayat merged commit b5484d2 into main Oct 14, 2022
@cexbrayat cexbrayat deleted the xanf-add-prepare-script branch October 14, 2022 08:41
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

3 participants