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

perf: remove test files from the package #1406

Merged
merged 6 commits into from
Oct 12, 2020
Merged

perf: remove test files from the package #1406

merged 6 commits into from
Oct 12, 2020

Conversation

RobertAron
Copy link
Contributor

Corresponding issue (if exists):

#979

What would you like to add/fix?

Prevent the test files from being published to NPM.

I'm not super comfortable with how lerna works, but I believe this is what's needed.

Todo

  • Add tests if possible (N/A)
  • Add changelog if users should know about the change (N/A)
  • Add documentation (N/A)

@RobertAron RobertAron requested a review from HenriBeck as a code owner October 10, 2020 21:20
@@ -17,6 +17,7 @@
"url": "https://github.com/cssinjs/jss/issues/new"
},
"files": [
"!*.test.js",
Copy link
Member

Choose a reason for hiding this comment

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

files is actually defining what should be in the package, we don't need to define what should not be in the package in addition

Copy link
Member

Choose a reason for hiding this comment

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

oh you are talking about the tests which are next to the src files

Copy link
Member

Choose a reason for hiding this comment

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

I wonder as well if that works, because it's a negative match, so it will probably just do nothing and move on to src and still match everything

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 tried with npm pack and thought it was working but just double-checked and I'm not so sure anymore...

I'll fiddle with it a bit more.

Copy link
Member

Choose a reason for hiding this comment

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

otherwise I only see .npmignore as an option

Copy link
Contributor Author

@RobertAron RobertAron Oct 10, 2020

Choose a reason for hiding this comment

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

I was able to glob ignore specifically the files that end in .test.js that are in the src folders. I could setup .npmignore instead if you think that's a cleaner approach.

Edit: actually I think this removed some of the other files though. I'll continue to look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty confident it's in a good state now. Here are the cases I was thinking about.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that glob looks scary, at this point I think .npmignore is cleaner

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 agree it's a little scary 😬

Apparently files included with the “package.json#files” field cannot be excluded through .npmignore or .gitignore. I wasn't sure if you meant to pull the files out of the package.json so I kept looking for a better solution.

I found someone suggested doing what I was doing originally so I went back to looking into it. Apparently what I did works, but you have to do it in a specific order. Let me know if that's an ok solution.

Also that link I included above mentions that files called LICENSE should be included no matter what you do, so I think it could be taken out of the array. For now, I left it in just in case there was some reason for it to be there.

@kof
Copy link
Member

kof commented Oct 11, 2020 via email

@kof
Copy link
Member

kof commented Oct 12, 2020

checked, it works, thank you

@kof kof merged commit bf89ac9 into cssinjs:master Oct 12, 2020
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.

2 participants