Skip to content

Use package.json files array instead of .npmignore - #50408

Merged
Jake Bailey (jakebailey) merged 1 commit into
microsoft:mainfrom
jakebailey:remove-npmignore
Aug 23, 2022
Merged

Use package.json files array instead of .npmignore#50408
Jake Bailey (jakebailey) merged 1 commit into
microsoft:mainfrom
jakebailey:remove-npmignore

Conversation

@jakebailey

@jakebailey Jake Bailey (jakebailey) commented Aug 22, 2022

Copy link
Copy Markdown
Member

In #50327, I forgot to add .eslintcache to .npmignore, meaning depending on how the build is run, this file may have appeared in our output package. This was fixed in #50380.

Rather than needing to remember to add any new file to .npmignore, it's (in my opinion) a lot safer to instead explicitly declare which files we want to be shipping instead. You can do that by using "files" in package.json; the meanings are reversed to .npmignore, but you can still write ignores via the ! prefix.

The result is the same as our current .npmignore, with a few exceptions:

  • lib/README.md is not explicitly ignored. This was in our .npmignore, except that it never worked because npm always includes files named README. See: https://unpkg.com/browse/typescript@4.7.4/lib/README.md
  • CopyrightNotice.txt: This is not the same as our license; it's just a file that we prepend to our outputs to add a header and should really be in some build folder. LICENSE.txt is our license. (When we move to modules, I plan to use copyright headers and a lint rule, so this file will disappear anyway.)
  • CODE_OF_CONDUCT.md, which we were required to add by policy in Added Microsoft code of conduct file #27581, but doesn't need to be in our published package and was likely overlooked similarly to my .eslintcache file.

The downside is that if you're adding a new file and it's not in one of the expected locations, it won't be included. But, I think it's unlikely that we add files in different locations, compared to very likely that we add more things like config files for tooling.

Comment thread package.json
"./bin",
"./lib",
"!./lib/enu",
"./loc",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should loc be shipped? What is it for?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 Nathan Shively-Sanders (@sandersn), CSIGS@microsoft.com (@csigs), or pdostalek (@PDostalek) know.

@jakebailey Jake Bailey (jakebailey) Aug 22, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

Comment thread package.json
Comment thread package.json
"files": [
"./bin",
"./lib",
"!./lib/enu",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have any idea what enu is?

@jakebailey Jake Bailey (jakebailey) Aug 22, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now, I'm not actually sure that this is copied in the lkg build; I will check.

(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.)

Comment thread package.json
"node": ">=4.2.0"
},
"files": [
"./bin",

@jakebailey Jake Bailey (jakebailey) Aug 22, 2022

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member Author

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.)

@jakebailey
Jake Bailey (jakebailey) merged commit 6ee5db9 into microsoft:main Aug 23, 2022
@jakebailey
Jake Bailey (jakebailey) deleted the remove-npmignore branch August 23, 2022 21:14
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants