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

feat(#6): added files field to package.json template #23

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

alemagio
Copy link
Contributor

@alemagio alemagio commented Jul 13, 2020

Description

I didn't add the "index.js" because it should be added automatically since it is the main file (npm doc)

Types of changes

Fixes #6

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

  • I have updated the documentation (if required).
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I added a picture of a cute animal cause it's fun

image

@alemagio
Copy link
Contributor Author

Do you think I should add any test for this scenario?

@lirantal
Copy link
Owner

Nah, what bothered me was the superficial src/ directory and then const app = require('./src) felt weird because it's kind of determining something before the code is there.

That said, I think it's an overall ok to begin with as a skeleton.
Will merge away. Thanks @alemagio 🙏

@lirantal lirantal merged commit eb9ad44 into lirantal:master Jul 16, 2020
@lirantal
Copy link
Owner

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lirantal
Copy link
Owner

@alemagio looks like this PR broke a few things like it's not including rootDir files such as .prettierrc.js and others (travis, etc). Another small bug is that the require for 'src' should actually be a local reference: require('./src').

Are you up to PRing the fixes?

@alemagio
Copy link
Contributor Author

Sure
I'll push it asap
Sorry for the mistake

@lirantal
Copy link
Owner

No worries, I also didn't notice at first 😉

@lirantal
Copy link
Owner

lirantal commented Jul 17, 2020

Also, no rush if it's weekend/family time for you.

@alemagio
Copy link
Contributor Author

I should have time this evening after work or tomorrow morning 😉

@lirantal
Copy link
Owner

Sounds good :-)

@alemagio
Copy link
Contributor Author

alemagio commented Jul 17, 2020

@alemagio looks like this PR broke a few things like it's not including rootDir files such as .prettierrc.js and others (travis, etc)

I have a question about this @lirantal .
I was reading the npm doc and, for what I understood, files like .prettierrc.js and travis.yml should not be included since those are for development. I would add SECURITY file maybe.
Please tell me if I'm wrong 😅

@lirantal
Copy link
Owner

@alemagio You're correct and I know what's happening now.
Here is the tree view of the package published in the previous version that worked: https://unpkg.com/browse/create-node-lib@2.3.3/template/ and here it is with the new version introducing the change this PR: https://unpkg.com/browse/create-node-lib@2.4.0/template/

You can see that it was packaged without these files to begin with, and you're correct about not distributing them as well.

What I think we can do is the following:

  1. Remove the src from package.json
  2. Use this trick https://github.com/lirantal/create-node-lib/blob/master/saofile.js#L84-L93 to update the package.json with that src/ entry in files which will happen when the package is scaffolded

@alemagio
Copy link
Contributor Author

Ok, got it.
It prevents those file to be in this package too, off course.
I'll fix it immediately.
Thanks for the explanation.

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.

Suggestion: add "files" to the package.json template
2 participants