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

Only necessary files are published to npm #4819

Merged
merged 2 commits into from
Jun 28, 2017
Merged

Only necessary files are published to npm #4819

merged 2 commits into from
Jun 28, 2017

Conversation

tomscholz
Copy link
Contributor

@mourner
Copy link
Member

mourner commented Jun 12, 2017

Thanks for the PR! Is there any advantage to specifying included files in package.json as opposed to excluded files in .npmignore? It seems to me that it would be much easier to accidentally screw up a release with the first approach by forgetting to update package.json after a change in file paths.

@DannyDelott
Copy link
Contributor

The general reason is that dotfiles are easy to miss when downloading/sharing source code or copying directories around.

There is also an argument to suggest that dotfiles are better for machine-specific config (eg: .git, .vimrc) and essential project config (eg: package.json, yarn.lock) belongs in files with more visibility.

package.json Outdated
"dist/",
"plugins/",
"LICENSE.txt",
"ARCHITECTURE.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'd want to include ARCHITECTURE.md in the npm package. We should include README.md and CHANGELOG.md though.

Also the plugins/ folder isn't needed (it actually shouldn't even be in master anymore)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Readme and changelog are automatically added see https://docs.npmjs.com/files/package.json#files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also remove the LICENSE.txt.
In the npm documentation it states

Certain files are always included, regardless of settings:
[...]
LICENSE / LICENCE
README, CHANGES, LICENSE & NOTICE can have any case and extension.

@mollymerp
Copy link
Contributor

You've still got /plugins in there 😸

@tomscholz
Copy link
Contributor Author

tomscholz commented Jun 17, 2017

Done! 💪🏻 @mollymerp

@mollymerp
Copy link
Contributor

mollymerp commented Jun 23, 2017

@mourner @anandthakker do you know if we need to include the flow_interfaces directory in the files published to the npm registry? From what I can tell flow is used primarily for testing and we unflowify before we build the dist files, but I'm not sure if we need the flow files for people using mapbox-gl with another bundler 💭

@anandthakker
Copy link
Contributor

do you know if we need to include the flow_interfaces directory in the files published to the npm registry? From what I can tell flow is used primarily for testing and we unflowify before we build the dist files, but I'm not sure if we need the flow files for people using mapbox-gl with another bundler

Yeah, IMO we should keep these in so that people who want to bundle our source directly have a fighting chance.

@jfirebaugh jfirebaugh merged commit 277e699 into mapbox:master Jun 28, 2017
@tomscholz tomscholz deleted the patch-npm-files branch July 1, 2017 13:08
@LitoMore
Copy link

LitoMore commented Feb 8, 2018

We should not publish the src to npm.

@anandthakker
Copy link
Contributor

@LitoMore we intentionally publish src to NPM so that developers who prefer to use flow-annotated sources are able to do so.

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.

7 participants