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

"build" scripts should be using "prepare" #519

Closed
jasonkarns opened this issue Mar 18, 2020 · 8 comments
Closed

"build" scripts should be using "prepare" #519

jasonkarns opened this issue Mar 18, 2020 · 8 comments
Labels
question Further information is requested

Comments

@jasonkarns
Copy link

Shouldn't all the examples be using the prepare script to build/bundle? That way packages are built correctly (and automatically) during packing, publishing, and when installed as git deps directly from github.

@fregante
Copy link
Contributor

prepack is the preferred way now

@jasonkarns
Copy link
Author

jasonkarns commented Nov 13, 2020

I don't believe that's true. Prepack doesn't run when a package is installed through git. Therefore for most builds that must be run to make a package usable, prepare is the appropriate hook. (Caveats abound, of course, but using prepack would disqualify installing packages from git)

edit I mistook part of the docs based on the lifecycle ordering. The docs do actually state that prepack will run when installing as a git dep.

@fregante So if it is a viable alternative, what is the rationale for prepack over prepare (or vice versa)?

@fregante
Copy link
Contributor

Never mind, honestly it's a mess. I'm sure that the docs suggested using prepack at some point but it doesn't really work correctly: npm/cli#1229 and maybe the behavior changed between npm 6 and 7.

So if it is a viable alternative, what is the rationale for prepack over prepare (or vice versa)?

The advantage of prepack is that:

  • it runs on npm pack as well, while prepare doesn't (npm pack --dry-run for example is useful for example to verify that what you'll publish is correct)
  • in the context of npm publish, prepack makes sense because it's the last possible moment before the tar file is generated

So for the time being if you need git installs you'll have to keep using prepare.

@jasonkarns
Copy link
Author

it runs on npm pack as well, while prepare doesn't (npm pack --dry-run for example is useful for example to verify that what you'll publish is correct)

Hmmm, I'm now doubting my own sanity (or npm's). I was reasonably confident that prepare did run during pack, because I've also used npm pack --dry-run heavily. Though it's been over a year now since my npm usage was "daily". So now I'm unsure if I'm mis-remembering, or if npm's behavior has changed. (And it's even odds between those choices!)

@fregante
Copy link
Contributor

Yup, it's not us, it's them.

I just tested it:

  • npm6 pack runs prepare
  • npm7 pack does not
❯ npm -v
6.14.8

❯ npm pack

> content-scripts-register-polyfill@2.0.0 prepare /Volumes/Base/Users/rico/Web/projects-modules/content-scripts-register-polyfill
> tsc --sourceMap false

npm notice 
npm notice 📦  content-scripts-register-polyfill@2.0.0
npm notice === Tarball Contents === 
npm notice 1.1kB license     
npm notice 3.0kB index.js    
npm notice 1.1kB package.json
npm notice 2.5kB readme.md   
npm notice 360B  globals.d.ts
npm notice 49B   index.d.ts  
npm notice === Tarball Details === 
npm notice name:          content-scripts-register-polyfill          
npm notice version:       2.0.0                                      
npm notice filename:      content-scripts-register-polyfill-2.0.0.tgz
npm notice package size:  3.3 kB                                     
npm notice unpacked size: 8.2 kB                                     
npm notice shasum:        4f9696fc998a9e2b190ea7fbe33ef05d1726f32a   
npm notice integrity:     sha512-7nQqZ+Kla7/h9[...]0QjALvfEdxyiA==   
npm notice total files:   6                                          
npm notice 
content-scripts-register-polyfill-2.0.0.tgz
❯ npm -v
7.0.14

❯ npm pack
npm notice 
npm notice 📦  content-scripts-register-polyfill@2.0.0
npm notice === Tarball Contents === 
npm notice 360B  globals.d.ts
npm notice 49B   index.d.ts  
npm notice 3.0kB index.js    
npm notice 1.1kB license     
npm notice 1.1kB package.json
npm notice 2.5kB readme.md   
npm notice === Tarball Details === 
npm notice name:          content-scripts-register-polyfill          
npm notice version:       2.0.0                                      
npm notice filename:      content-scripts-register-polyfill-2.0.0.tgz
npm notice package size:  3.3 kB                                     
npm notice unpacked size: 8.2 kB                                     
npm notice shasum:        8dd0d6780e9398f754f3968d6ff63d7b8c083482   
npm notice integrity:     sha512-J8XN9lnVtULwi[...]bztQO1WmSHI+g==   
npm notice total files:   6                                          
npm notice 
content-scripts-register-polyfill-2.0.0.tgz

@fregante
Copy link
Contributor

fregante commented Jan 21, 2021

Also I just tried installing something from GitHub and:

  • npm7 install fregante/webext-patterns runs prepack correctly, including the dev dependencies
  • npm7 install fregante/select-dom stalls for 4 minutes and then silently fails

The prepack step is just $ tsc in both

@styfle styfle added the question Further information is requested label Oct 18, 2023
@styfle
Copy link
Member

styfle commented Oct 18, 2023

Closing since this is a choice that each project gets to make.

For example, I often have prepublishOnly call build and build call ncc.

@styfle styfle closed this as not planned Won't fix, can't repro, duplicate, stale Oct 18, 2023
@jasonkarns
Copy link
Author

Agreed that each project can make their own choice. My suggestion here is for the docs and examples to impart some knowledge. Many/most node devs I've encountered have no idea that there are OOTB lifecycle hooks tailor-made for "building". And so in the wild, most projects will define a build script out of naivety, not realizing that in so doing, their package is now useless as a git-dep.

The suggestion here is that the docs can model more useful defaults (since most devs will just copy/paste from these examples) and end up with a more useful package without realizing it. And the eagle-eyed might just learn something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants