Skip to content

Conversation

@sorenlouv
Copy link
Member

Currently it is not possible to import json files in typescript. In APM we've worked around this by adding a declaration file, that declares json files as any. This is sub-optimal, since we lose all the type info available in a static config.

Enabling resolveJsonModule will let devs import .json as they .import other files. It only works when module: "commonjs".


const tsConfigPath = resolve(projectFlag);
return PROJECTS.filter(project => project.tsConfigPath === tsConfigPath);
return PROJECTS.filter(project => project.tsConfigPath.includes(tsConfigPath));
Copy link
Member Author

Choose a reason for hiding this comment

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

I found it useful to be able to run tslint for x-pack like this:
node scripts/tslint --project x-pack

Is that okay, or should I revert this change?

Copy link
Contributor

@spalger spalger Nov 13, 2018

Choose a reason for hiding this comment

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

I'd be fine with something a little bit stricter, maybe:

project.tsConfigPath === tsConfigPath || project.tsConfigPath === resolve(tsConfigPath, 'tsconfig.json')

Pretty sure as is --project . will match any tsconfig.json file, depending on which one is first.

"test/**/*"
],
"compilerOptions": {
"resolveJsonModule": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

Initially added this to kibana/tsconfig.json but it caused problems in kbn-pm/tsconfig.json and kibana/tsconfig.browser.json since they do not emit commonjs. I can probably override this with "resolveJsonModule": false if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you wouldn't mind setting this in the root config and negating it where necessary that would keep the compilerOptions a little more consistent across projects, which is desirable.

@sorenlouv
Copy link
Member Author

sorenlouv commented Nov 11, 2018

The build is currently failing. I can repro the issue locally by runing npx tsc in kibana/x-pack:

error TS5055: Cannot write file '/Users/sqren/elastic/kibana/x-pack/plugins/apm/public/components/shared/PropertiesTable/propertyConfig.json' because it would overwrite input file.

error TS5055: Cannot write file '/Users/sqren/elastic/kibana/x-pack/plugins/beats_management/server/utils/index_templates/beats_template.json' because it would overwrite input file.

error TS5055: Cannot write file '/Users/sqren/elastic/kibana/x-pack/plugins/infra/common/graphql/introspection.json' because it would overwrite input file.

error TS5055: Cannot write file '/Users/sqren/elastic/kibana/x-pack/plugins/spaces/mappings.json' because it would overwrite input file.

@spalger or anyone else: any good ideas for fixing the above?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor

spalger commented Nov 13, 2018

@sqren that's a tricky one... We rely on the TypeScript being written in place right now and moving away from that will be pretty tricky to get working in the build process, but I also don't know of any way to force TS to write the output or to not emit .json files. I wonder if we could avoid using the actual CLI and take over emitting the results, skipping over the .json files?

@sorenlouv
Copy link
Member Author

sorenlouv commented Nov 19, 2018

I wonder if we could avoid using the actual CLI and take over emitting the results, skipping over the .json files?

That sounds like a good approach, and I might do that in the future if no-one beats me to it. However, for now I'll close this issue. I'll outline the reason below.

Workaround
As a temporary workaround I converted .json to .ts which works.
A disadvantage of json that I had not considered previously, is the inability to annotate with types. This causes problems like this:

data.json

[{ "value": 0 }, { "value": 1 }, { "value": "NaN" }]

filterItems.ts

import { data } from './data.json'; // will be of type: ({ value: number | string })[]

interface Item {
  value: number | 'NaN';
}

function filterItems(items: Item[]) {
  return items.filter(item => item.value !== 'NaN');
}

filterItems(data); // ERROR: Type 'string' is not assignable to type 'number | "NaN"'

Above fails because typescript infers the list of values as number | string, where we actually want the more specific number | 'NaN'

That issue goes away when annotating with types:

data.ts

import { Item } from './Item'
export const data: Item[] = [{ value: 0 }, { value: 1 }, { value: 'NaN' }];

filterItems.ts

import { data } from './data.ts'; // will be of type: ({ value: number | 'NaN' })[]

interface Item {
  value: number | 'NaN';
}

function filterItems(items: Item[]) {
  return items.filter(item => item.value !== 'NaN');
}

filterItems(data); // No errors :)

There are of course still advantages to being able to import json but for now it doesn't seem worth the hassle.

@sorenlouv sorenlouv closed this Nov 19, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

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