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

tsconfig: extend from node_modules #731

Closed
wants to merge 1 commit into from

Conversation

pjonsson
Copy link
Contributor

Depends on: #729

There is no "terriajs" directory
in this repository, so import
tsconfig.json from the terriajs
directory inside node_modules
instead, just like the ESLint
configuration does.

This requires re-defining outDir,
typeRoots, and include to the same
values that they have in terriajs,
since they will otherwise be relative
to the node_modules tsconfig.json.

The types option requires including
a couple of lib/ThirdParty directories
in terriajs.

The maxNodeModuleJsDepth parameter
is required for not getting errors
about implicit any types on jsx
imports in terriajs source code.

There is no "terriajs" directory
in this repository, so import
tsconfig.json from the terriajs
directory inside node_modules
instead, just like the ESLint
configuration does.

This requires re-defining outDir,
typeRoots, and include to the same
values that they have in terriajs,
since they will otherwise be relative
to the node_modules tsconfig.json.

The types option requires including
a couple of lib/ThirdParty directories
in terriajs.

The maxNodeModuleJsDepth parameter
is required for not getting errors
about implicit any types on jsx
imports in terriajs source code.
@zoran995
Copy link
Collaborator

There is no need for this, as typescript will use node resolution to resolve the file, and having it use a relative path might cause issues for others using Terriamap in a non-standard way (someone installed terriamap and terriajs as dep, it will put terriajs at different place and break it).

@zoran995 zoran995 closed this Mar 27, 2025
@pjonsson
Copy link
Contributor Author

There is no need for this

How can the type error fixed by #729 pass the CI if this isn't needed?

@zoran995
Copy link
Collaborator

I don't understand what kind of type error you get in #729 in CI; from what I see, that PR passes the CI.
There is no point in adding node_modules to extends as it will potentially create more issues than it has benefits. I can try looking into what's going on, but this is not the proper fix, at least from what I currently see

@pjonsson
Copy link
Contributor Author

I'm not saying that this PR is necessarily the right fix, but my point is that the function expects two parameters:

$ rg createPluginContext node_modules/terriajs-plugin-api/
node_modules/terriajs-plugin-api/dist/TerriaPlugin.d.ts
52:export declare function createPluginContext(viewState: ViewState, pluginConfig: any): TerriaPluginContext;

and that isn't caught by the CI. I also don't get a type error when I run yarn run tsc -b tsconfig.json in a fresh clone of the TerriaMap repository.

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