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
Use package.json files array instead of .npmignore #50408
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,17 @@ | |
"engines": { | ||
"node": ">=4.2.0" | ||
}, | ||
"files": [ | ||
"./bin", | ||
"./lib", | ||
"!./lib/enu", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any idea what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe these are the XML files that the localization team consumes. Our localize task produces both the output we use as well as these files to give to them to use in some GUI program, so if we don't ignore them here, they'll appear in the package which isn't desirable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now, I'm not actually sure that this is copied in the (Personally, I think that these localization team things would be better suited for a different task, as we don't need it in our dev workflow day-to-day, but that's a topic for another PR.) |
||
"./loc", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I honestly have no clue; it's already in the package we publish. Maybe that's an oversight too! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did a quick by-hand bisect and loc was first added to the package in 3.7; I have yet to go look why. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it needs to be shipped. It is likely part of the configuration for our localization team. Maybe @sandersn, @csigs, or @PDostalek know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing it shaves off 78 files (almost half) and 2.7 MB (!) unpacked, so removing it would be great. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact, this folder hasn't been touched in 3 years; does it still contain anything that's in use? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After talking in standup, I'm going to send a PR that deletes this folder altogether from the repo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern with that is a possible situation where some infra breaks and we stop getting localization updates. But the PR itself is a good place to discuss further. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, for now, this PR keeps the status quo except the limited number of exceptions I listed above. |
||
"./AUTHORS.md", | ||
"./LICENSE.txt", | ||
DanielRosenwasser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"./README.md", | ||
"./SECURITY.md", | ||
"./ThirdPartyNoticeText.txt" | ||
], | ||
"devDependencies": { | ||
"@octokit/rest": "latest", | ||
"@types/async": "latest", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made these all
./
, but that's not actually required.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(But, I'll probably leave it that way, as it mirrors the other paths we use in the file; see above.)