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

chore(deps): bump @readme/httpsnippet from 6.2.1 to 11.0.0 #1091 #1182

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AndriiAndreiev
Copy link
Collaborator

🧰 Changes

Bumps @readme/httpsnippet from 6.2.1 to 11.0.0.

🧬 QA & Testing

Fixed failing tests

dependabot bot and others added 4 commits October 1, 2024 19:20
Bumps [@readme/httpsnippet](https://github.com/readmeio/httpsnippet) from 6.2.1 to 11.0.0.
- [Release notes](https://github.com/readmeio/httpsnippet/releases)
- [Commits](readmeio/httpsnippet@6.2.1...11.0.0)

---
updated-dependencies:
- dependency-name: "@readme/httpsnippet"
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

thank you for this! i have a few small tsconfig-related questions but otherwise LGTM!

@@ -1,5 +1,6 @@
{
"compilerOptions": {
"moduleResolution": "nodenext",
Copy link
Member

@kanadgupta kanadgupta Feb 28, 2025

Choose a reason for hiding this comment

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

typescript will be changing the behavior of NodeNext in 5.8 so i'd recommend pinning it like so:

Suggested change
"moduleResolution": "nodenext",
"moduleResolution": "Node16",

Comment on lines +4 to +5
"module": "nodenext",
"moduleResolution": "nodenext",
Copy link
Member

Choose a reason for hiding this comment

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

can we remove these lines? i believe this can be inferred from the tsconfig.build.json

Suggested change
"module": "nodenext",
"moduleResolution": "nodenext",

Copy link
Member

Choose a reason for hiding this comment

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

@domharrington @erunion any idea why we have two separate tsconfig files in this subpackage? any reason why we can't merge them?

Copy link
Member

Choose a reason for hiding this comment

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

this package is so old, i have no idea.

@erunion
Copy link
Member

erunion commented Feb 28, 2025

@azinder1 were we still planning on moving this package into the monorepo?

@kanadgupta
Copy link
Member

@erunion spoke with @azinder1 offline and here's the sequencing i'm imagining:

  • publish a one final breaking release of sdk-snippets with the changes i made in #1190
  • upgrade it in readme and make sure everything works as expected
  • if so, we deprecate the sdk-snippets package on npm and migrate the code over to the monorepo

i think we can close this PR in favor of #1190

let me know what you think of all of this @gratcliff!

@erunion
Copy link
Member

erunion commented Mar 4, 2025

i'm fine with that, but tbqh i don't think i'd bother with publishing a new release of that package. nobody but us should be using it as we're the only ones that get any value out of it. it also has dependents or published forks.

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