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

fix: make ts-node dependency truly optional #1825

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jun 15, 2024

What did you implement:

ts-node is meant to be an opt-in dependency (as in, a dependency that is not installed by default, but will be used if installed) per #636, but it's been added as an optional dependency rather than an .. err ... "right kind of optional dependency" (aka peer dependency set as optional).

In the context of package.json, "optional dependencies" are ones that the package manager should try to install but skip if they error - they're intended for dependencies that are os-specific like fsevents or the underlying binaries for esbuild - so right now, ts-node will always be installed because it does not have any os constraints, and in turn it leads to complaints about missing peer dependencies via ts-node:

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object. This is a map of package name to version or URL, just like the dependencies object. The difference is that build failures do not cause installation to fail.

In the context of package.json, the correct way to express "optional" in the way most people think is to include the dependency as a peer dependency with optional: true in its meta settings.

specifically, it allows peer dependencies to be marked as optional. Npm will not automatically install optional peer dependencies. This allows you to integrate and interact with a variety of host packages without requiring all of them to be installed.

How did you implement it:

I added ts-node as an optional peer dependency, and removed typescript and @types/node as peer dependencies since they're not used directly by serverless-webpack

How can we verify it:

Install serverless-webpack, do an npm ls and see that ts-node isn't installed nor are there any dependency warnings - you can then install typescript and ts-node, and you'll still not have any dependency warnings.

Also see serverless/serverless-azure-functions#517 for prior art

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: Yes
Is it a breaking change?: Debatable 😅

@vicary vicary requested a review from j0k3r June 15, 2024 05:08
@j0k3r
Copy link
Member

j0k3r commented Jun 18, 2024

Is it a breaking change?: Debatable 😅

Well I do think people using TS with serverless-webpack already have ts-node as deps (at least we do). So moving it from a truly optional deps looks legit to me!

@G-Rath
Copy link
Contributor Author

G-Rath commented Jun 18, 2024

Well I do think people using TS with serverless-webpack already have ts-node as deps

Yup exactly my thoughts too

@j0k3r j0k3r merged commit 778338e into serverless-heaven:master Jun 18, 2024
14 checks passed
@G-Rath G-Rath deleted the ts-node-optional branch June 18, 2024 07:06
@j0k3r j0k3r added this to the 5.14.1 milestone Jun 18, 2024
@j0k3r j0k3r mentioned this pull request Jun 18, 2024
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.

2 participants