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.

Sign up for GitHub

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

Maintenance: Fix @ts-expect-error strict types #20981

Merged
merged 3 commits into from
Feb 17, 2023
Merged

Maintenance: Fix @ts-expect-error strict types #20981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d16356
chore(typescript): fix `@ts-expect-error` strict types
sheriffMoose Feb 7, 2023
c1d9f2b
fix(typescript): strict types should not throw errors
sheriffMoose Feb 7, 2023
93feb6e
Merge branch 'next' into strict-types
ndelangen Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions code/frameworks/angular/template/stories/argTypes/doc-button/doc-button.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ export class DocButtonComponent<T> {

/** Test null default value. */
@Input()
// @ts-expect-error intentionally undefined
public anUndefinedValue;
public anUndefinedValue: undefined;

/** Test numeric default value. */
@Input()
Expand Down
33 changes: 18 additions & 15 deletions code/lib/csf-tools/src/ConfigFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const _getPath = (path: string[], node: t.Node): t.Node | undefined => {
}
if (t.isObjectExpression(node)) {
const [first, ...rest] = path;
const field = node.properties.find((p: t.ObjectProperty) => propKey(p) === first);
const field = (node.properties as t.ObjectProperty[]).find((p) => propKey(p) === first);
if (field) {
return _getPath(rest, (field as t.ObjectProperty).value);
}
Expand All @@ -41,7 +41,7 @@ const _getPathProperties = (path: string[], node: t.Node): t.ObjectProperty[] |
}
if (t.isObjectExpression(node)) {
const [first, ...rest] = path;
const field = node.properties.find((p: t.ObjectProperty) => propKey(p) === first);
const field = (node.properties as t.ObjectProperty[]).find((p) => propKey(p) === first);
if (field) {
// FXIME handle spread etc.
if (rest.length === 0) return node.properties as t.ObjectProperty[];
Expand Down Expand Up @@ -92,8 +92,8 @@ const _makeObjectExpression = (path: string[], value: t.Expression): t.Expressio
// eslint-disable-next-line @typescript-eslint/naming-convention
const _updateExportNode = (path: string[], expr: t.Expression, existing: t.ObjectExpression) => {
const [first, ...rest] = path;
const existingField = existing.properties.find(
(p: t.ObjectProperty) => propKey(p) === first
const existingField = (existing.properties as t.ObjectProperty[]).find(
(p) => propKey(p) === first
) as t.ObjectProperty;
if (!existingField) {
existing.properties.push(
Expand All @@ -113,7 +113,7 @@ export class ConfigFile {

_exports: Record<string, t.Expression> = {};

_exportsObject: t.ObjectExpression;
_exportsObject: t.ObjectExpression | undefined;

_quotes: 'single' | 'double' | undefined;

Expand Down Expand Up @@ -144,12 +144,12 @@ export class ConfigFile {

if (t.isObjectExpression(decl)) {
self._exportsObject = decl;
decl.properties.forEach((p: t.ObjectProperty) => {
(decl.properties as t.ObjectProperty[]).forEach((p) => {
const exportName = propKey(p);
if (exportName) {
let exportVal = p.value;
if (t.isIdentifier(exportVal)) {
exportVal = _findVarInitialization(exportVal.name, parent as t.Program);
exportVal = _findVarInitialization(exportVal.name, parent as t.Program) as any;
}
self._exports[exportName] = exportVal as t.Expression;
}
Expand All @@ -166,9 +166,9 @@ export class ConfigFile {
node.declaration.declarations.forEach((decl) => {
if (t.isVariableDeclarator(decl) && t.isIdentifier(decl.id)) {
const { name: exportName } = decl.id;
let exportVal = decl.init;
let exportVal = decl.init as t.Expression;
if (t.isIdentifier(exportVal)) {
exportVal = _findVarInitialization(exportVal.name, parent as t.Program);
exportVal = _findVarInitialization(exportVal.name, parent as t.Program) as any;
}
self._exports[exportName] = exportVal;
}
Expand All @@ -191,16 +191,19 @@ export class ConfigFile {
) {
let exportObject = right;
if (t.isIdentifier(right)) {
exportObject = _findVarInitialization(right.name, parent as t.Program);
exportObject = _findVarInitialization(right.name, parent as t.Program) as any;
}
if (t.isObjectExpression(exportObject)) {
self._exportsObject = exportObject;
exportObject.properties.forEach((p: t.ObjectProperty) => {
(exportObject.properties as t.ObjectProperty[]).forEach((p) => {
const exportName = propKey(p);
if (exportName) {
let exportVal = p.value;
let exportVal = p.value as t.Expression;
Copy link
Member

@shilman shilman Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need all these extra type annotations? Is it causing a problem without?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, #20877

Copy link
Contributor Author

@sheriffMoose sheriffMoose Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@valentinpalkovic & I are affected, can't do changes without disabling strict mode and do few changes

if (t.isIdentifier(exportVal)) {
exportVal = _findVarInitialization(exportVal.name, parent as t.Program);
exportVal = _findVarInitialization(
exportVal.name,
parent as t.Program
) as any;
}
self._exports[exportName] = exportVal as t.Expression;
}
Expand Down Expand Up @@ -311,7 +314,7 @@ export class ConfigFile {

const pathNames: string[] = [];
if (t.isArrayExpression(node)) {
node.elements.forEach((element) => {
(node.elements as t.Expression[]).forEach((element) => {
pathNames.push(this._getPresetValue(element, 'name'));
});
}
Expand Down Expand Up @@ -414,7 +417,7 @@ export class ConfigFile {

const properties = this.getFieldProperties(path) as t.ObjectProperty[];
if (properties) {
const lastPath = path.at(-1);
const lastPath = path.at(-1) as string;
removeProperty(properties, lastPath);
}
}
Expand Down
45 changes: 22 additions & 23 deletions code/lib/csf-tools/src/CsfFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ function parseTags(prop: t.Node) {
}

const findVarInitialization = (identifier: string, program: t.Program) => {
let init: t.Expression = null;
let declarations: t.VariableDeclarator[] = null;
let init: t.Expression = null as any;
let declarations: t.VariableDeclarator[] = null as any;
program.body.find((node: t.Node) => {
if (t.isVariableDeclaration(node)) {
declarations = node.declarations;
Expand All @@ -57,7 +57,7 @@ const findVarInitialization = (identifier: string, program: t.Program) => {
t.isIdentifier(decl.id) &&
decl.id.name === identifier
) {
init = decl.init;
init = decl.init as t.Expression;
return true; // stop looking
}
return false;
Expand All @@ -68,7 +68,7 @@ const findVarInitialization = (identifier: string, program: t.Program) => {
};

const formatLocation = (node: t.Node, fileName?: string) => {
const { line, column } = node.loc.start;
const { line, column } = node.loc?.start || {};
return `${fileName || ''} (line ${line}, col ${column})`.trim();
};

Expand Down Expand Up @@ -108,7 +108,7 @@ const isArgsStory = (init: t.Node, parent: t.Node, csf: CsfFile) => {

const parseExportsOrder = (init: t.Expression) => {
if (t.isArrayExpression(init)) {
return init.elements.map((item: t.Expression) => {
return (init.elements as t.Expression[]).map((item) => {
if (t.isStringLiteral(item)) {
return item.value;
}
Expand Down Expand Up @@ -185,7 +185,7 @@ export class CsfFile {

constructor(ast: t.File, { fileName, makeTitle }: CsfOptions) {
this._ast = ast;
this._fileName = fileName;
this._fileName = fileName as string;
this.imports = [];
this._makeTitle = makeTitle;
}
Expand All @@ -204,15 +204,14 @@ export class CsfFile {

_parseMeta(declaration: t.ObjectExpression, program: t.Program) {
const meta: StaticMeta = {};
declaration.properties.forEach((p: t.ObjectProperty) => {
(declaration.properties as t.ObjectProperty[]).forEach((p) => {
if (t.isIdentifier(p.key)) {
this._metaAnnotations[p.key.name] = p.value;

if (p.key.name === 'title') {
meta.title = this._parseTitle(p.value);
} else if (['includeStories', 'excludeStories'].includes(p.key.name)) {
// @ts-expect-error (Converted from ts-ignore)
meta[p.key.name] = parseIncludeExclude(p.value);
(meta as any)[p.key.name] = parseIncludeExclude(p.value);
} else if (p.key.name === 'component') {
const { code } = generate.default(p.value, {});
meta.component = code;
Expand All @@ -236,7 +235,7 @@ export class CsfFile {

getStoryExport(key: string) {
let node = this._storyExports[key] as t.Node;
node = t.isVariableDeclarator(node) ? node.init : node;
node = t.isVariableDeclarator(node) ? (node.init as t.Node) : node;
if (t.isCallExpression(node)) {
const { callee, arguments: bindArguments } = node;
if (
Expand All @@ -262,7 +261,7 @@ export class CsfFile {
traverse.default(this._ast, {
ExportDefaultDeclaration: {
enter({ node, parent }) {
let metaNode: t.ObjectExpression;
let metaNode: t.ObjectExpression | undefined;
const isVariableReference = t.isIdentifier(node.declaration) && t.isProgram(parent);
let decl;
if (isVariableReference) {
Expand All @@ -277,9 +276,9 @@ export class CsfFile {
t.isVariableDeclaration(topLevelNode) &&
topLevelNode.declarations.find(isVariableDeclarator)
);
decl = (self._metaStatement as t.VariableDeclaration).declarations.find(
decl = ((self?._metaStatement as t.VariableDeclaration)?.declarations || []).find(
isVariableDeclarator
).init;
)?.init;
} else {
self._metaStatement = node;
decl = node.declaration;
Expand Down Expand Up @@ -316,7 +315,7 @@ export class CsfFile {
if (t.isIdentifier(decl.id)) {
const { name: exportName } = decl.id;
if (exportName === '__namedExportsOrder' && t.isVariableDeclarator(decl)) {
self._namedExportsOrder = parseExportsOrder(decl.init);
self._namedExportsOrder = parseExportsOrder(decl.init as t.Expression);
return;
}
self._storyExports[exportName] = decl;
Expand All @@ -334,7 +333,7 @@ export class CsfFile {
// eslint-disable-next-line @typescript-eslint/naming-convention
let __isArgsStory = true; // assume default render is an args story
// CSF3 object export
decl.init.properties.forEach((p: t.ObjectProperty) => {
(decl.init.properties as t.ObjectProperty[]).forEach((p) => {
if (t.isIdentifier(p.key)) {
if (p.key.name === 'render') {
__isArgsStory = isArgsStory(p.value as t.Expression, parent, self);
Expand All @@ -354,7 +353,7 @@ export class CsfFile {
parameters = {
// __id: toId(self._meta.title, name),
// FIXME: Template.bind({});
__isArgsStory: isArgsStory(fn, parent, self),
__isArgsStory: isArgsStory(fn as t.Node, parent, self),
};
}
self._stories[exportName] = {
Expand All @@ -370,7 +369,7 @@ export class CsfFile {
if (t.isExportSpecifier(specifier) && t.isIdentifier(specifier.exported)) {
const { name: exportName } = specifier.exported;
if (exportName === 'default') {
let metaNode: t.ObjectExpression;
let metaNode: t.ObjectExpression | undefined;
const decl = t.isProgram(parent)
? findVarInitialization(specifier.local.name, parent)
: specifier.local;
Expand Down Expand Up @@ -418,7 +417,7 @@ export class CsfFile {

if (self._storyAnnotations[exportName]) {
if (annotationKey === 'story' && t.isObjectExpression(annotationValue)) {
annotationValue.properties.forEach((prop: t.ObjectProperty) => {
(annotationValue.properties as t.ObjectProperty[]).forEach((prop) => {
if (t.isIdentifier(prop.key)) {
self._storyAnnotations[exportName][prop.key.name] = prop.value;
}
Expand Down Expand Up @@ -475,13 +474,13 @@ export class CsfFile {

// default export can come at any point in the file, so we do this post processing last
const entries = Object.entries(self._stories);
self._meta.title = this._makeTitle(self._meta.title);
self._meta.title = this._makeTitle(self._meta?.title as string);
if (self._metaAnnotations.play) {
self._meta.tags = [...(self._meta.tags || []), 'play-fn'];
}
self._stories = entries.reduce((acc, [key, story]) => {
if (isExportStory(key, self._meta)) {
const id = toId(self._meta.id || self._meta.title, storyNameFromExport(key));
if (isExportStory(key, self._meta as StaticMeta)) {
const id = toId((self._meta?.id || self._meta?.title) as string, storyNameFromExport(key));
const parameters: Record<string, any> = { ...story.parameters, __id: id };
const { includeStories } = self._meta || {};
if (
Expand All @@ -506,7 +505,7 @@ export class CsfFile {
}, {} as Record<string, StaticStory>);

Object.keys(self._storyExports).forEach((key) => {
if (!isExportStory(key, self._meta)) {
if (!isExportStory(key, self._meta as StaticMeta)) {
delete self._storyExports[key];
delete self._storyAnnotations[key];
}
Expand Down Expand Up @@ -557,5 +556,5 @@ export const readCsf = async (fileName: string, options: CsfOptions) => {
export const writeCsf = async (csf: CsfFile, fileName?: string) => {
const fname = fileName || csf._fileName;
if (!fname) throw new Error('Please specify a fileName for writeCsf');
await fs.writeFile(fileName, await formatCsf(csf));
await fs.writeFile(fileName as string, await formatCsf(csf));
};
2 changes: 1 addition & 1 deletion code/lib/csf-tools/src/enrichCsf.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ export const enrichCsf = (csf: CsfFile, options?: EnrichCsfOptions) => {

export const extractSource = (node: t.Node) => {
const src = t.isVariableDeclarator(node) ? node.init : node;
const { code } = generate.default(src, {});
const { code } = generate.default(src as t.Node, {});
return code;
};

Expand Down
12 changes: 6 additions & 6 deletions code/lib/csf-tools/src/getStorySortParameter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import { babelParse } from './babelParse';
const logger = console;

const getValue = (obj: t.ObjectExpression, key: string) => {
let value: t.Expression;
obj.properties.forEach((p: t.ObjectProperty) => {
let value: t.Expression | undefined;
(obj.properties as t.ObjectProperty[]).forEach((p) => {
if (t.isIdentifier(p.key) && p.key.name === key) {
value = p.value as t.Expression;
}
Expand All @@ -20,12 +20,12 @@ const getValue = (obj: t.ObjectExpression, key: string) => {

const parseValue = (expr: t.Expression): any => {
if (t.isArrayExpression(expr)) {
return expr.elements.map((o: t.Expression) => {
return (expr.elements as t.Expression[]).map((o) => {
return parseValue(o);
});
}
if (t.isObjectExpression(expr)) {
return expr.properties.reduce((acc, p: t.ObjectProperty) => {
return (expr.properties as t.ObjectProperty[]).reduce((acc, p) => {
if (t.isIdentifier(p.key)) {
acc[p.key.name] = parseValue(p.value as t.Expression);
}
Expand Down Expand Up @@ -60,7 +60,7 @@ const unsupported = (unexpectedVar: string, isError: boolean) => {
};

export const getStorySortParameter = (previewCode: string) => {
let storySort: t.Expression;
let storySort: t.Expression | undefined;
const ast = babelParse(previewCode);
traverse.default(ast, {
ExportNamedDeclaration: {
Expand Down Expand Up @@ -109,7 +109,7 @@ export const getStorySortParameter = (previewCode: string) => {

if (t.isFunctionExpression(storySort)) {
const { code: sortCode } = generate.default(storySort, {});
const functionName = storySort.id.name;
const functionName = storySort.id?.name;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should add a TODO comment here to remind to take a look at this code.
if storySort.id can be undefined, it means that the code right after will generate invalid code, e.g.:

return undefined(a, b)

Copy link
Member

Choose a reason for hiding this comment

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

@sheriffMoose @ndelangen seems like this PR got merged but this was taken into account. Do you think this might bite us back if we don't at least add a TODO there?

Copy link
Member

Choose a reason for hiding this comment

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

If you feel like there should be a comment, and you have an idea what that comment should look like, sure.
I don't have a recollection of this context anymore.

Copy link
Member

Choose a reason for hiding this comment

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

The (now merged) code looks like this:

if (t.isFunctionExpression(storySort)) {
    const { code: sortCode } = generate.default(storySort, {});
    const functionName = storySort.id?.name;
    // Wrap the function within an arrow function, call it, and return
    const wrapper = `(a, b) => {
      ${sortCode};
      return ${functionName}(a, b)
    }`;
    // eslint-disable-next-line no-eval
    return (0, eval)(wrapper);
  }

so if storySort.id is optional, then storySort.id?.name could be undefined, right? meaning that functionName becomes undefined.

later on, return ${functionName}(a, b) will get executed. But then, if functionName is undefined, the evaluated code will be undefined(a,b), which will crash.

// Wrap the function within an arrow function, call it, and return
const wrapper = `(a, b) => {
${sortCode};
Expand Down