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(pkg): explicitly include files for release #349

Merged
merged 1 commit into from
Mar 22, 2018

Conversation

rgbkrk
Copy link
Contributor

@rgbkrk rgbkrk commented Mar 15, 2018

What kind of change does this PR introduce?

Build related change

Did you add tests for your changes?

N/A

If relevant, did you update the documentation?

N/A

Summary

When running flow in a project that uses webpack-cli, the latest release is causing flow to choke on a non-valid json file:

> flow

Launching Flow server for /Users/kylek/code/src/github.com/nteract/nteract-ext
Spawned flow server (pid=10402)
Logs will go to /private/tmp/flow/zSUserszSkylekzScodezSsrczSgit.luolix.topzSnteractzSnteract-ext.log
Monitor logs will go to /private/tmp/flow/zSUserszSkylekzScodezSsrczSgit.luolix.topzSnteractzSnteract-ext.monitor_log
Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/webpack-cli/.nyc_output/8f27269b188e61a59bbf5827de818c30.json:1:1

Unexpected end of input

     1│

Since that file shouldn't be part of the release, I added the .nyc_output directory to the npm ignore.

Does this PR introduce a breaking change?

Nope.

@jsf-clabot
Copy link

jsf-clabot commented Mar 15, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

Please sign CLA and use commitlint for linting commit messages. You can use git cz for an interactive commit or use one of these.

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Thanks!

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Mar 15, 2018

@dhruvdutt would you say this is a chore or a fix?

@dhruvdutt
Copy link
Member

chore

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Heyo, didn't read this. Seems to be another PR for this #354 . Could you do the thing I mentioned there? First one to implement gets a lgtm 😄

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Mar 20, 2018

Rebased with requested commit style.

@dhruvdutt
Copy link
Member

@rgbkrk Do check what Even has mentioned in his review on #354

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Mar 20, 2018

Yeah I just added on the ones @ev1stensberg explicitly mentioned, looking into what else is misc though I'm sure you'll be able to rattle off which you don't want more easily as a maintainer.

Do you want docs in the packaged release?

Looking again, jsdoc.json was already in there, so I'm rebasing again.

Copy link
Member

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

LGTM.

@dhruvdutt
Copy link
Member

dhruvdutt commented Mar 20, 2018

@rgbkrk docs are already included in npmignore file.
You can also clean up things from npmignore that are already included in .gitignore like .vscode

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Mar 20, 2018

Alright, I've organized the npmignore more like the gitignore.

Copy link
Member

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

Uh! I might not have explained it in the best way.
I meant to clean up or not include things that are already listed in gitignore. For instance, .vscode is currently mentioned in both npmignore and gitignore but if it's in gitignore then it won't be in version control or on GitHub in the first place hence we don't need to mention it in npmignore.

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Mar 20, 2018

Do you publish from a developer laptop or from CI?

@dhruvdutt
Copy link
Member

IMO we publish manually. You can see the list of npm publishers here.

@ooflorent
Copy link
Member

Why not using files field from package.json? It would simplify the maintenance.

.npmignore Outdated

# Miscellany
Copy link
Member

Choose a reason for hiding this comment

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

Consider revising this

@webpack-bot
Copy link

@rgbkrk Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ev1stensberg Please review the new changes.

@rgbkrk rgbkrk changed the title Don't upload .nyc_output on release chore(pkg): explicitly include files for release Mar 21, 2018
@rgbkrk
Copy link
Contributor Author

rgbkrk commented Mar 21, 2018

Switched this over to using files in the package.json. Note that README.md and certain other files are automatically included and do not have to be explicitly stated in files.

Copy link
Member

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

LGTM.

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Mar 21, 2018

Just to follow up, even though we're using files now:

Do you publish from a developer laptop or from CI?

IMO we publish manually. You can see the list of npm publishers here.

Since a .npmignore overrides what's in a .gitignore, if a developer has files locally that shouldn't be in the package yet are git ignored, they'll accidentally upload them as part of the package. Some files npm makes sure to avoid .DS_Store, swap files, etc. Since that's a bit of a hairy mess to figure out continuously, the files approach is definitely better.

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Amazing job! Thanks!!

@evenstensberg evenstensberg merged commit c3a0c9f into webpack:master Mar 22, 2018
@evenstensberg
Copy link
Member

webpack-cli @2.0.13

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Mar 22, 2018

Thanks!

@evenstensberg
Copy link
Member

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants