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

Add script to fix package json from build step #410

Merged
merged 2 commits into from Mar 3, 2023

Conversation

oscard0m
Copy link
Member

@oscard0m oscard0m commented Mar 2, 2023

Resolves #405


Behavior

Before the change?

The released npm package is missing most of the files generated by the build step. dist-node, dist-types, dist-web... even though they are generated correctly.

You can read more about my explanation in the linked issue.

After the change?

I expected npm to read the file patterns correctly so we publish all the necessary files again.

Other information

This is a mix of an issue with npm@v9 (npm/cli#6164) and the fact we rely on pika for the build step. Pika has been archived since April 2022 so there is nothing we can do with Pika.

I'm opening a discussion to discuss what we should do: octokit/octokit.js#2403

Open questions

If we agree on this solution, we need to plan:

  • How to merge and release this?
  • What do we do with the rest of the repositories?
  • What do we do with the already published versions with missing files?

Additional info

Pull request checklist

Because this is kind of a temporary hack, do you think I should add tests + documentation for this?

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

No

Pull request type

Because of the problems is giving to users, I'm treating it as a bug: Type: Bug. In terms of semantic commit, let me know if I need to change ci() to fix()


@ghost ghost added this to Inbox in JS Mar 2, 2023
@oscard0m oscard0m added the Type: Bug Something isn't working as documented label Mar 2, 2023
@ghost ghost moved this from Inbox to Bugs in JS Mar 2, 2023
@oscard0m oscard0m requested review from gr2m, kfcampbell, nickfloyd and a team March 2, 2023 16:56
Copy link
Member

@wolfy1339 wolfy1339 left a comment

Choose a reason for hiding this comment

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

LGTM only one small code style issue

scripts/fix-package-json.js Outdated Show resolved Hide resolved
@oscard0m
Copy link
Member Author

oscard0m commented Mar 2, 2023

LGTM only one small code style issue

  • How to merge and release this?
  • What do we do with the rest of the repositories?
  • What do we do with the already published versions with missing files?

@wolfy1339
Copy link
Member

  • How to merge and release this?
    Merge this as a bugfix release
  • What do we do with the rest of the repositories?

I suppose we would need to apply the fix there as well

  • What do we do with the already published versions with missing files?

Those versions could be marked as deprecated

@oscard0m oscard0m force-pushed the add-script-to-fix-package-json-from-build-step branch from 4d03afa to ac01115 Compare March 2, 2023 17:18
@oscard0m oscard0m requested a review from wolfy1339 March 2, 2023 17:19
@oscard0m oscard0m requested a review from wolfy1339 March 2, 2023 17:31
@oscard0m oscard0m force-pushed the add-script-to-fix-package-json-from-build-step branch from ac01115 to eea9cfb Compare March 2, 2023 17:32
@oscard0m oscard0m requested review from wolfy1339 and removed request for wolfy1339 March 3, 2023 13:37
@wolfy1339 wolfy1339 merged commit 7549659 into main Mar 3, 2023
JS automation moved this from Bugs to Done Mar 3, 2023
@wolfy1339 wolfy1339 deleted the add-script-to-fix-package-json-from-build-step branch March 3, 2023 22:59
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

🎉 This PR is included in version 4.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wolfy1339
Copy link
Member

@oscard0m Can you run this through an octoherd script to apply this to all the repositories? Until we get esbuild done so releases aren't broken.

The following repos had recent releases:

  • @octokit/app
  • @octokit/oauth-app

@oscard0m
Copy link
Member Author

@oscard0m Can you run this through an octoherd script to apply this to all the repositories? Until we get esbuild done so releases aren't broken.

The following repos had recent releases:

  • @octokit/app
  • @octokit/oauth-app

Sure! I will try to make some time during the weekend, but I'm unsure if I can. I'll keep you posted.

@oscard0m
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Bug Something isn't working as documented
Projects
Archived in project
JS
  
Done
Development

Successfully merging this pull request may close these issues.

[BUG]: dist-types/index.d.ts is not present in the published NPM package
2 participants