search

strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Add npmignore (test/examples aren't needed in npm) #232

Closed STRML closed 8 years ago

bajtos commented 9 years ago

@STRML thank you for the pull request.

I think this change is more complex than it looks on the first sight. AFAIK, when the package provides .npmignore file, npm publish uses only that file and ignores .gitignore entries. Therefore we should add most/all gitignore entries to npmignore too, otherwise we may end up publishing things like coverage reports or swap files to npmjs, which is IMO worse that publishing tests & examples.

I am also not sure if the benefits of not publishing tests & examples is worth the extra maintenance effort needed to keep gitignore and npmignore in sync.

Thoughts?

/cc @raymondfeng @ritch

STRML commented 9 years ago

That's a good point, and it's unfortunate that there isn't a switch in npmignore to import the gitignore.

The tests/examples are about 180kB zipped. It's a decent savings.

clarkbw commented 9 years ago

Apologies for the random drive by comment here but I just wanted to add in my experience. I wish the code @STRML wrote would work because that's exactly how I think developers actually want npmignore to work. Merge to the two ignore files to minimize the npm module footprint. Leaving aside the arguments that including example and test code are nice to have for developers when offline.

However npmignore doesn't support merging but instead overrides the gitignore file. You do get a lot of files ignored for free in the npmignore so it would not have to include everything you have in your .gitignore but then you're still maintaining that difference against the npmignore list. For my projects the hassle usually outweighs the savings.

In this case I think you do have a decent size of tests files so it might be worth it to ignore them from npm, maybe if checking the difference were automated into tests or an npm prepublish script.

STRML commented 9 years ago

I agree, maybe we just do a simple contains check in one of the tests so we can catch this.

It would be great if npmignore had some special syntax (like @.gitignore) to import another file. Maybe a task for someone with free time? On Aug 7, 2015 12:01 AM, "Bryan Clark" notifications@github.com wrote:

Apologies for the random drive by comment here but I just wanted to add in my experience. I wish the code @STRML https://github.com/STRML wrote would work because that's exactly how I think developers actually want npmignore to work. Merge to the two ignore files to minimize the npm module footprint. Leaving aside the arguments that including example and test code are nice to have for developers when offline.

However npmignore doesn't support merging https://docs.npmjs.com/misc/developers#keeping-files-out-of-your-package but instead overrides the gitignore file. You do get a lot of files ignored for free in the npmignore so it would not have to include everything you have in your .gitignore https://github.com/strongloop/strong-remoting/blob/master/.gitignore but then you're still maintaining that difference against the npmignore list. For my projects the hassle usually outweighs the savings.

In this case I think you do have a decent size of tests files so it might be worth it to ignore them from npm, maybe if checking the difference were automated into tests or an npm prepublish script.

— Reply to this email directly or view it on GitHub https://github.com/strongloop/strong-remoting/pull/232#issuecomment-128597250 .

bajtos commented 9 years ago

@STRML I agree, maybe we just do a simple contains check in one of the tests so we can catch this.

Sounds reasonable. Could you please improve this patch to include such test?

bajtos commented 8 years ago

I am closing this pull request as it looks pretty much abandoned.