Skip to content

Commit

Permalink
fix(types): Generate maps to allow improved "go to definition" behavi…
Browse files Browse the repository at this point in the history
…our (#9269)

**Problem**
Navigating to the definition of pages, layouts, components, cells etc.
would lead you to the .d.ts file. This can be really frustrating if
you're used to flying around between definitions this way.

**Changes**
1. Generates basic definition/source mappings for:
    1. directory mapped modules
    2. cell mirrors
    3. router pages
    4. route links

**Linked issues**
Fixes #5862
Fixes #2867 

**Performance**
This adds additional AST parsing work to the `yarn rw g types` so it has
the potential to slow that command down. I have tested that command on
the test project and the results are:

Benchmark: `hyperfine --runs 32 --warmup 3 'yarn rw g types'`
Main:
```
Benchmark 1: yarn rw g types
  Time (mean ± σ):      4.006 s ±  0.048 s    [User: 5.244 s, System: 0.737 s]
  Range (min … max):    3.926 s …  4.171 s    32 runs
```
PR:
```
Benchmark 1: yarn rw g types
  Time (mean ± σ):      4.629 s ±  0.049 s    [User: 6.066 s, System: 0.828 s]
  Range (min … max):    4.544 s …  4.723 s    32 runs
```
An approximate +15% change in duration on this basic test.

There are areas where caching results could improve performance. For
example we are likely finding the location of page components twice when
we could cache the first result. I have not added this here so we do not
prematurely optimise and introduce subtle bugs that would be more
difficult to track down. If users with large projects report problems
with this performance change then we can work to address it.

**Notes**
This is not an area I'm particularly knowledgeable about so there could
be unknown consequences to this that I don't know about.

For pages and other web components it's easy to direct the map to the
definition of the specific component that is the default export. This is
not the case for cells as they have no default export and instead have a
number of other non-default exports. In this case I currently direct the
map to the head of the source file - happy to improve on this based on
feedback.

For future reference I found the following useful:
* https://www.murzwin.com/base64vlq.html
*
https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.1ce2c87bpj24
  • Loading branch information
Josh-Walker-GM authored and jtoar committed Oct 10, 2023
1 parent 1a90c0b commit 3294114
Show file tree
Hide file tree
Showing 12 changed files with 302 additions and 8 deletions.
1 change: 1 addition & 0 deletions packages/internal/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"kill-port": "1.6.1",
"prettier": "2.8.8",
"rimraf": "5.0.1",
"source-map": "0.7.4",
"string-env-interpolation": "1.0.1",
"systeminformation": "5.21.7",
"terminal-link": "2.1.1",
Expand Down
24 changes: 24 additions & 0 deletions packages/internal/src/__tests__/ast.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,26 +37,50 @@ test('extracts named exports', () => {
expect(n).toMatchInlineSnapshot(`
[
{
"location": {
"column": 9,
"line": 1,
},
"name": "exportA",
"type": "re-export",
},
{
"location": {
"column": 18,
"line": 1,
},
"name": "exportB",
"type": "re-export",
},
{
"location": {
"column": 13,
"line": 3,
},
"name": "myVariableExport",
"type": "variable",
},
{
"location": {
"column": 13,
"line": 5,
},
"name": "myArrowFunctionExport",
"type": "variable",
},
{
"location": {
"column": 16,
"line": 9,
},
"name": "myFunctionExport",
"type": "function",
},
{
"location": {
"column": 13,
"line": 11,
},
"name": "MyClassExport",
"type": "class",
},
Expand Down
20 changes: 20 additions & 0 deletions packages/internal/src/__tests__/jsx.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ test('simple jsx tree', () => {
"children": [
{
"children": [],
"location": {
"column": 8,
"line": 5,
},
"name": "Route",
"props": {
"name": "home",
Expand All @@ -27,6 +31,10 @@ test('simple jsx tree', () => {
},
{
"children": [],
"location": {
"column": 8,
"line": 6,
},
"name": "Route",
"props": {
"name": "login",
Expand All @@ -36,6 +44,10 @@ test('simple jsx tree', () => {
},
{
"children": [],
"location": {
"column": 8,
"line": 7,
},
"name": "Route",
"props": {
"name": "404",
Expand All @@ -44,12 +56,20 @@ test('simple jsx tree', () => {
},
},
],
"location": {
"column": 6,
"line": 4,
},
"name": "Set",
"props": {
"private": true,
},
},
],
"location": {
"column": 4,
"line": 3,
},
"name": "Router",
"props": {},
},
Expand Down
4 changes: 4 additions & 0 deletions packages/internal/src/__tests__/typeDefinitions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ test('generate the correct mirror types for cells', () => {
type CellInputs = CellProps<SuccessType, NumTodosCell_GetCount, typeof Cell, NumTodosCell_GetCountVariables>
export default function (props: CellInputs): ReturnType<SuccessType>
//# sourceMappingURL=index.d.ts.map
"
`)
})
Expand Down Expand Up @@ -95,6 +97,8 @@ test('generate the correct mirror types for directory named modules', () => {
import { default as DEFAULT } from './requireAuth'
export default DEFAULT
export * from './requireAuth'
//# sourceMappingURL=index.d.ts.map
"
`)
})
Expand Down
66 changes: 66 additions & 0 deletions packages/internal/src/ast.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ export const fileToAst = (filePath: string): types.Node => {
interface NamedExports {
name: string
type: 're-export' | 'variable' | 'function' | 'class'
location: {
line: number
column: number
}
}
/**
* get all the named exports in a given piece of code.
Expand All @@ -58,6 +62,10 @@ export const getNamedExports = (ast: types.Node): NamedExports[] => {
namedExports.push({
name: id.name,
type: 're-export',
location: {
line: id.loc?.start.line ?? 1,
column: id.loc?.start.column ?? 0,
},
})
}
return
Expand All @@ -73,16 +81,28 @@ export const getNamedExports = (ast: types.Node): NamedExports[] => {
namedExports.push({
name: id.name as string,
type: 'variable',
location: {
line: id.loc?.start.line ?? 1,
column: id.loc?.start.column ?? 0,
},
})
} else if (declaration.type === 'FunctionDeclaration') {
namedExports.push({
name: declaration?.id?.name as string,
type: 'function',
location: {
line: declaration?.id?.loc?.start.line ?? 1,
column: declaration?.id?.loc?.start.column ?? 0,
},
})
} else if (declaration.type === 'ClassDeclaration') {
namedExports.push({
name: declaration?.id?.name as string,
type: 'class',
location: {
line: declaration?.id?.loc?.start.line ?? 1,
column: declaration?.id?.loc?.start.column ?? 0,
},
})
}
},
Expand Down Expand Up @@ -148,3 +168,49 @@ export const hasDefaultExport = (ast: types.Node): boolean => {
})
return exported
}

export const getDefaultExportLocation = (
ast: types.Node
): { line: number; column: number } | null => {
// Get the default export
let defaultExport: types.ExportDefaultDeclaration | undefined
traverse(ast, {
ExportDefaultDeclaration(path) {
defaultExport = path.node
},
})

if (!defaultExport) {
return null
}

// Handle the case were we're exporting a variable declared elsewhere
// as we will want to find the location of that declaration instead
if (types.isIdentifier(defaultExport.declaration) && types.isFile(ast)) {
// Directly search the program body for the declaration of the identifier
// to avoid picking up other identifiers with the same name in the file
const exportedName = defaultExport.declaration.name
const declaration = ast.program.body.find((node) => {
return (
types.isVariableDeclaration(node) &&
node.declarations.find((d) => {
return (
types.isVariableDeclarator(d) &&
types.isIdentifier(d.id) &&
d.id.name === exportedName
)
})
)
})

return {
line: declaration?.loc?.start.line ?? 1,
column: declaration?.loc?.start.column ?? 0,
}
}

return {
line: defaultExport.loc?.start.line ?? 1,
column: defaultExport.loc?.start.column ?? 0,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ export * from './${name}'
type CellInputs = CellProps<SuccessType, ${queryResultType}, typeof Cell, ${queryVariablesType}>

export default function (props: CellInputs): ReturnType<SuccessType>

//# sourceMappingURL=index.d.ts.map
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { default as DEFAULT } from './${name}'
export default DEFAULT
export * from './${name}'

//# sourceMappingURL=index.d.ts.map
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ ${routes.map(
}
}

//# sourceMappingURL=web-routerRoutes.d.ts.map
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ ${pages.map(({ importName }) => {
return ` const ${importName}: typeof ${importName}Type`
}).join('\n')}
}

//# sourceMappingURL=web-routesPages.d.ts.map
Loading

0 comments on commit 3294114

Please sign in to comment.