WIP: Grouped types #60
Conversation
| const relativePath = path.node.source.value; | ||
| // convert relative path to absolute | ||
| const absolutePath = resolveDependencyPath({ dependency: relativePath, entryAST: fileAST, entryPath: filePath }); | ||
| const specifiers = [ |
There was a problem hiding this comment.
mb add some NOTE with sens like "there could be some other specifiers in file with the same import path"?
| const dependencies = resolveDependencies({ entryAST: fileAST }); | ||
|
|
||
| // TBD: which identifier should we use in case path is missing? | ||
| storage.set('__source', { dependencies, fileAST }); |
There was a problem hiding this comment.
__source seems OK. But mb move to constants
| entries.forEach(entry => { | ||
| const filePath = path.resolve(entry); | ||
|
|
||
| if (!storage.has(filePath)) { |
There was a problem hiding this comment.
Mb use early return? (some descussion of why it could be better -https://softwareengineering.stackexchange.com/q/18454/11854)
| if (!storage.has(filePath)) { | |
| if (storage.has(filePath)) { | |
| return; | |
| } |
| const interfaceFile = state.dependenciesTree.get(absoluteFilePath); | ||
|
|
||
| if (interfaceFile) { | ||
| const importName = t.isImportDefaultSpecifier(importSpecifier) ? |
There was a problem hiding this comment.
Looks like this logic could be moved to the place where specifiers are extracted.
Not sure if we need some other info here except specifier name in this file.
There was a problem hiding this comment.
I had an idea in mind that buildDependenciesTree is responsible for building the tree itself, without knowing which content might be used in future for markdown generation process.
But maybe it's possible to put interfaceVisitor logic inside createApiClassMethod / createApiClassProperty / createApiFunction methods?
| // NOTE: it might be the case when more than 1 import exists. | ||
| // Example: | ||
| // import { MyType } from './types'; | ||
| // import { MyType } from './another-type'; |
There was a problem hiding this comment.
Is not it invalid code if there are two imports with the same specifier name?
There was a problem hiding this comment.
Hm, I was thinking such sort of validation have to be done inside eslint / tslint tools.
But it's not true - typescript is throwing an error in such cases TS2300: Duplicate identifier 'Type'.
But in the other hand next code should be valid:
// @ts-ignore
import { Type } from './type1.ts';
// @ts-ignore
import { Type } from './type2.ts';
| createApiClassDeclaration(path.node, path, options), | ||
| ); | ||
| traverseInterfaces(path, state); |
There was a problem hiding this comment.
mb better to traverse a node, returned by createApi[ClassDeclaration|ClassMethod|..]?
| ...createApiVisitor((path, options) => { | ||
| }; | ||
|
|
||
| const traverseInterfaces = (path, state) => path.traverse(interfaceVisitor, state); |
There was a problem hiding this comment.
As for me traverseInterfaces name is too abstract.
Mb something like traversNodeTypes?
There was a problem hiding this comment.
as for me traversNodeTypes even more abstract, anyways it's already changed to
path.traverse(interfaceVisitor, { filePath });
| // NOTE: it might be the case when more than 1 import exists. | ||
| // Example: | ||
| // import { MyType } from './types'; | ||
| // import { MyType } from './another-type'; |
There was a problem hiding this comment.
Hm, I was thinking such sort of validation have to be done inside eslint / tslint tools.
But it's not true - typescript is throwing an error in such cases TS2300: Duplicate identifier 'Type'.
But in the other hand next code should be valid:
// @ts-ignore
import { Type } from './type1.ts';
// @ts-ignore
import { Type } from './type2.ts';
| const visitors = { | ||
| enter: (path, state) => { | ||
| //store filePath locally in order to be able | ||
| filePath = state.filePath; |
| ...createApiVisitor((path, options) => { | ||
| }; | ||
|
|
||
| const traverseInterfaces = (path, state) => path.traverse(interfaceVisitor, state); |
There was a problem hiding this comment.
as for me traversNodeTypes even more abstract, anyways it's already changed to
path.traverse(interfaceVisitor, { filePath });
| }, | ||
| // In order to implement CommonJS support can use https:/dependents/node-detective-cjs | ||
| ImportDeclaration: path => { | ||
| // TODO: prevent traverse for files without filePath in more efficient and elegant way |
No description provided.