-
Notifications
You must be signed in to change notification settings - Fork 139
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]: Make TSConfig stricter and fix as many errors as possible #1596
Open
inlined
wants to merge
7
commits into
main
Choose a base branch
from
inlined.strict
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Doing a social network crawl, I've managed to get ahold of someone who could fix the long library. This should fix the firebase, google-cloud, and langchain plugins. Will extend my tests to verify. |
Welp; langchain produced a lot of errors still: node_modules/@langchain/core/dist/language_models/chat_models.d.ts:75:129 - error TS2304: Cannot find name 'this'.
75 _generateCached({ messages, cache, llmStringKey, parsedOptions, handledOptions, }: ChatModelGenerateCachedParameters<typeof this>): Promise<LLMResult & {
~~~~
node_modules/@langchain/core/dist/language_models/llms.d.ts:67:129 - error TS2304: Cannot find name 'this'.
67 _generateCached({ prompts, cache, llmStringKey, parsedOptions, handledOptions, runId, }: LLMGenerateCachedParameters<typeof this>): Promise<LLMResult & {
~~~~
node_modules/@langchain/core/tracers/base.d.cts:1:15 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("../dist/tracers/base.js")' call instead.
1 export * from '../dist/tracers/base.js'
~~~~~~~~~~~~~~~~~~~~~~~~~
node_modules/@langchain/openai/dist/azure/chat_models.d.ts:2:10 - error TS2305: Module '"@langchain/core/language_models/chat_models"' has no exported member 'LangSmithParams'.
2 import { LangSmithParams, type BaseChatModelParams } from "@langchain/core/language_models/chat_models";
~~~~~~~~~~~~~~~
node_modules/@langchain/openai/dist/chat_models.d.ts:6:25 - error TS2305: Module '"@langchain/core/language_models/chat_models"' has no exported member 'LangSmithParams'.
6 import { BaseChatModel, LangSmithParams, type BaseChatModelParams } from "@langchain/core/language_models/chat_models";
~~~~~~~~~~~~~~~
node_modules/langchain/evaluation.d.cts:1:15 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("./dist/evaluation/index.js")' call instead.
1 export * from './dist/evaluation/index.js'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I got firebase and google-cloud to work, though I had to kick some lockfiles. It might be worth someone else double checking my work. New code added: import { enableFirebaseTelemetry } from "@genkit-ai/firebase";
import { enableGoogleCloudTelemetry } from "@genkit-ai/google-cloud";
enableFirebaseTelemetry();
enableGoogleCloudTelemetry(); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part of #1591 I've added a bit more stringency to Genkit's tsconfigs. This is required to add a genkit dependency to
firebase-functions
as it currently compiles cleanly withskipLibCheck: false
explicitly in customers' default templates; asking customers to change this would be a breaking change.Common fixes included removing unused or non-existent imports/exports, removing dead code, typing the results of
response.json()
, adding missing libraries, and in one case adding a missing return statement. I started by modifying the base tsconfig.json and then reenabling (only)skipLibCheck
on packages that are broken due to downstream dependencies, with a comment describing the reason why the setting is necessary. As a follow-up, we can file bugs and/or make contributions to those downstream libraries to further lessen our dependency onskipLibCheck
.This change ensures
skipLibCheck
is not necessary for the following packages:This change does not fix the following pacakges:
Issues with the remaining packages:
firebase, google-cloud, langchain are all blocked on the fact that "long" (a dependency of protobufjs) doesn't do import/require correctlyEDIT firebase and google-cloud were fixed with an update to thelong
library, but langchain has a smattering of errors listed below.method
type isnever
. We have four errors because ts-up is turning our type imports into normal imports and dropping thewith { 'resolution-mode': 'import' }
annotation, which breaks the CJS .d.ts. I've confirmed that editing the output lib file fixes the issue but it doesn't seem ts-up can be made to preserve imports correctly.Checklist (if applicable):
Tested by verifying the following program compiles as both a commonJS and ESM module: