Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

how come vendor directory is ignored in package.json? #1183

Closed
gkatsev opened this issue Sep 29, 2015 · 10 comments
Closed

how come vendor directory is ignored in package.json? #1183

gkatsev opened this issue Sep 29, 2015 · 10 comments
Labels
Feature - Request

Comments

@gkatsev
Copy link
Contributor

gkatsev commented Sep 29, 2015

I'm using node-sass in a build project of sorts which gets packaged on a CI/CD server using npm pack. However, because the package.json is ignoring it in the files array npm pack isn't including it in the output tarball.
Our CI runs on the same architecture of the prod machine, so, it should just work if the binary were to be included.

Thanks!
@saper
Copy link
Member

saper commented Sep 30, 2015

We have decided not to include binaries in the npm package because currently there are 49 of them, weighing total 409 MB. With hopefully more to come!

@gkatsev
Copy link
Contributor Author

gkatsev commented Sep 30, 2015

I understand not including the binaries in the package. My issue is that when I package up my app which depends on node-sass, node-sass ends up not including the downloaded binaries and my app breaks.
I'm not asking for the binaries to be distributed with node-sass but rather when that when I run npm pack on my package, node-sass currently-downloaded-binary will get included in the generated tarball. This way, I won't be required to run npm rebuild when running my app in my production server.
Does this make sense? If not, I'll try and make an example to make it clearer.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 1, 2015

@gkatsev we understand your issue. Do you have a suggested fix that would not result in us publishing the binaries to npm?

@gkatsev
Copy link
Contributor Author

gkatsev commented Oct 1, 2015

Including vendor in the package.json is necessary. Then, a prepublish script can make sure to delete the vendor directory before npm publish node-sass so it doesn't accidentally get added to a release of node-sass. And then when a dependent npm packs their app, the vendor directory gets added in their package and not ignored. I've just tested this out locally and seems to be working as expected.
I can PR this if that sounds like a good plan. Otherwise, I'll try and think of what other options are available.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 2, 2015

Sounds good. We'd be happy to see a PR.

@xzyfer xzyfer added this to the next.patch milestone Oct 2, 2015
@gkatsev
Copy link
Contributor Author

gkatsev commented Oct 14, 2015

I haven't forgotten about this. Been away last week, hope to get a PR out this week.

@gkatsev gkatsev mentioned this issue Dec 22, 2015
Closed
@gkatsev
Copy link
Contributor Author

gkatsev commented Feb 12, 2016

So, I've started finally looking at this in detail. Unfortunately, prepublish runs on both npm install and npm publish, which means that if you clone this repo and run npm install with the current change, it will download/build the binary but then it will run prepublish and the vendor directory will be removed.
Looking into making it run only on publish.

@gkatsev
Copy link
Contributor Author

gkatsev commented Feb 12, 2016

It's possible there isn't an answer here until npm/npm#10074 is resolved.

@gkatsev
Copy link
Contributor Author

gkatsev commented Feb 12, 2016

I think a workaround is available by using this module: https://github.com/iarna/in-publish

gkatsev added a commit to gkatsev/node-sass that referenced this issue Feb 16, 2016
@gkatsev
Remove vendor directory on prepublish and pack
c51b49b 
This is necessary because if a user is bundling node-sass in their
package, the vendor directory should stay so that the user isn't
required to run 'npm rebuild' afterwards, assuming that it was
originally installed on a like-architecture.
However, at the same time, when publishing, we don't want to include the
binaries since they may not work for a user plus there's the build step
that tries to download the correct binary for the current architecture.
Unfortunately, npm's prepublish hook runs on both prepublish *and* npm
install (see npm/npm#10074), so, using in-publish as a workaround to get
the prepublish script to only run prepublish and pack but not on
install.
This fixes sass#1183.
@gkatsev gkatsev mentioned this issue Feb 16, 2016
Closed
@gkatsev
Copy link
Contributor Author

gkatsev commented Feb 16, 2016

Sorry it's taken so long but I finally have a PR ready for this in #1384.

xzyfer pushed a commit to xzyfer/node-sass that referenced this issue Mar 23, 2016
@gkatsev @xzyfer
Remove vendor directory on prepublish and pack
0064482 
This is necessary because if a user is bundling node-sass in their
package, the vendor directory should stay so that the user isn't
required to run 'npm rebuild' afterwards, assuming that it was
originally installed on a like-architecture.

However, at the same time, when publishing, we don't want to include the
binaries since they may not work for a user plus there's the build step
that tries to download the correct binary for the current architecture.

Unfortunately, npm's prepublish hook runs on both prepublish *and* npm
install (see npm/npm#10074), so, using in-publish as a workaround to get
the prepublish script to only run prepublish and pack but not on
install.

Fixes sass#1183
Closes sass#1384
@xzyfer xzyfer mentioned this issue Mar 23, 2016
Merged
@xzyfer xzyfer modified the milestone: next.patch Sep 4, 2016
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this issue Apr 7, 2024
@mgreter
Merge pull request sass#1183 from mgreter/bugfix/issue_1016
5a089a0 
Fix parent selector interpolation in attribute selector
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Feature - Request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove vendor directory on prepublish and pack gkatsev/node-sass
3 participants
@saper @gkatsev @xzyfer