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

[fei5815.2.workaround] Add step to tidy up files #1024

Merged
merged 9 commits into from
Aug 30, 2024

Conversation

somewhatabstract
Copy link
Member

@somewhatabstract somewhatabstract commented Aug 29, 2024

Summary:

It seems that changesets publish isn't honoring our root .npmignore even though npm pack would. This works around that by borrowing some code from Wonder Blocks where we tidy up any ignored files that aren't source-controlled, reducing what we package.

We also explicitly include things in each package.json.

This combination means we can easily exclude files and folders in the dist location, without having really explicit files definitions that might be hard to maintain.

Issue: FEI-5815

Test plan:

yarn build:types and see the files being removed.

@somewhatabstract somewhatabstract self-assigned this Aug 29, 2024
@somewhatabstract somewhatabstract requested a review from a team August 29, 2024 22:19
Copy link

changeset-bot bot commented Aug 29, 2024

🦋 Changeset detected

Latest commit: 4dd9499

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@khanacademy/wonder-stuff-ci Patch
@khanacademy/wonder-stuff-core Patch
@khanacademy/wonder-stuff-i18n Patch
@khanacademy/wonder-stuff-render-environment-jsdom Patch
@khanacademy/wonder-stuff-render-server Patch
@khanacademy/wonder-stuff-sentry Patch
@khanacademy/wonder-stuff-server Patch
@khanacademy/wonder-stuff-testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@khan-actions-bot khan-actions-bot requested a review from a team August 29, 2024 22:19
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Aug 29, 2024

Gerald

Required Reviewers
  • @Khan/frontend-infra for changes to .gitignore, package.json, yarn.lock, .changeset/clean-apples-knock.md, build-settings/check-type-definitions.ts, packages/eslint-config-khan/.npmignore, packages/eslint-plugin-khan/.npmignore, packages/eslint-plugin-khan/package.json, packages/wonder-stuff-ci/.npmignore, packages/wonder-stuff-core/.npmignore, packages/wonder-stuff-i18n/.npmignore, packages/wonder-stuff-render-environment-jsdom/.npmignore, packages/wonder-stuff-render-server/.npmignore, packages/wonder-stuff-sentry/.npmignore, packages/wonder-stuff-sentry/package.json, packages/wonder-stuff-server/.npmignore, packages/wonder-stuff-testing/.npmignore

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (fd1083e) to head (4dd9499).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1024   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          97       97           
  Lines        1392     1392           
  Branches      351      358    +7     
=======================================
  Hits         1390     1390           
  Misses          1        1           
  Partials        1        1           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd1083e...4dd9499. Read the comment docs.

Copy link
Contributor

github-actions bot commented Aug 29, 2024

Size Change: 0 B

Total Size: 4.63 kB

ℹ️ View Unchanged
Filename Size
packages/wonder-stuff-core/dist/browser/es/index.js 1.85 kB
packages/wonder-stuff-sentry/dist/browser/es/index.js 1.65 kB
packages/wonder-stuff-testing/dist/browser/es/index.js 1.12 kB

compressed-size-action

@jeresig
Copy link
Member

jeresig commented Aug 30, 2024

What happens if we put a symlink to the .npmignore (or a copy of it) in each of the package directories? That would make sense that it wouldn't work for a global .npmignore and instead would be per-package. If that could work I'd definitely prefer that to a custom script.

@somewhatabstract
Copy link
Member Author

What happens if we put a symlink to the .npmignore (or a copy of it) in each of the package directories? That would make sense that it wouldn't work for a global .npmignore and instead would be per-package. If that could work I'd definitely prefer that to a custom script.

We already have the script - I just adapted it to use the .npmignore rather than hard-code all the things it deleted.

The issue with a symlinked .npmignore is getting the paths right. I think for that, we'd want the master file to not be a root .npmignore but rather some file like npm-ignore-template and then symlink that, so we're clear that the paths are not relative to root but relative to a package root.

I can give that a try. If that works then we can drop the script entirely rather than just edit it, then keep the files change and the type definitions check I added so that types are correct. This should reduce our package sizes significantly without breaking anything.

@somewhatabstract
Copy link
Member Author

OK, I tried the symlinks in Wonder Blocks too (which does a publish of pre-release packages, making this easier to test) and it completely fails.

🦋  error npm error path /home/runner/work/wonder-blocks/wonder-blocks/packages/wonder-blocks-toolbar/.npmignore

However, I think just having a copy of .npmignore in each folder and a script to overwrite with the template will provide an easy to update workaround. We could run the script manually when adding a package and just have a simple consistency test to catch when a package doesn't ahve one.

@somewhatabstract somewhatabstract merged commit a026cee into main Aug 30, 2024
8 checks passed
@somewhatabstract somewhatabstract deleted the fei5815.2.workaround branch August 30, 2024 19:56
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.

3 participants