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

Jump to bottom

fix(router-plugin): do not split if known ident is exported #2334

Merged
merged 11 commits into from
Sep 18, 2024
Merged

fix(router-plugin): do not split if known ident is exported #2334

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
51b3f3c
fix(router-plugin): do not split component if ident is being exported
SeanCassiere Sep 13, 2024
416ad59
fix(router-plugin): better logic for not splitting the component and …
SeanCassiere Sep 13, 2024
205b060
fix(router-plugin): handle the split file case
SeanCassiere Sep 14, 2024
fbf60c6
test(router-plugin): handling the component and loader being exported…
SeanCassiere Sep 14, 2024
1c82c28
fix(router-plugin): handle named exports that are functions
SeanCassiere Sep 14, 2024
919449e
fix(router-plugin): use an else-if
SeanCassiere Sep 14, 2024
b59a7b4
test(router-plugin): check that function exports are also being retained
SeanCassiere Sep 14, 2024
d890bad
refactor(router-plugin): simplified code-path for handling the proper…
SeanCassiere Sep 14, 2024
7eb0300
refactor(router-plugin): add a more descriptive warning message
SeanCassiere Sep 14, 2024
a50bb7a
test(router-plugin): add more test cases
SeanCassiere Sep 14, 2024
cf3d8d1
Merge branch 'main' into codesplitting-retain-default-exports
SeanCassiere Sep 15, 2024
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
278 changes: 213 additions & 65 deletions packages/router-plugin/src/core/code-splitter/compilers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,95 +98,118 @@ export function compileCodeSplitReferenceRoute(opts: ParseAstOptions) {
if (prop.key.name === 'component') {
const value = prop.value

let shouldSplit = true

if (t.isIdentifier(value)) {
existingCompImportPath =
getImportSpecifierAndPathFromLocalName(
programPath,
value.name,
).path

removeIdentifierLiteral(path, value)
}

// Prepend the import statement to the program along with the importer function
// Check to see if lazyRouteComponent is already imported before attempting
// to import it again
// exported identifiers should not be split
// since they are already being imported
// and need to be retained in the compiled file
const isExported = hasExport(ast, value)
shouldSplit = !isExported

if (
!hasImportedOrDefinedIdentifier(
'lazyRouteComponent',
)
) {
programPath.unshiftContainer('body', [
template.statement(
`import { lazyRouteComponent } from '@tanstack/react-router'`,
)(),
])
if (shouldSplit) {
removeIdentifierLiteral(path, value)
}
}

if (
!hasImportedOrDefinedIdentifier(
'$$splitComponentImporter',
)
) {
programPath.unshiftContainer('body', [
if (shouldSplit) {
// Prepend the import statement to the program along with the importer function
// Check to see if lazyRouteComponent is already imported before attempting
// to import it again

if (
!hasImportedOrDefinedIdentifier(
'lazyRouteComponent',
)
) {
programPath.unshiftContainer('body', [
template.statement(
`import { lazyRouteComponent } from '@tanstack/react-router'`,
)(),
])
}

if (
!hasImportedOrDefinedIdentifier(
'$$splitComponentImporter',
)
) {
programPath.unshiftContainer('body', [
template.statement(
`const $$splitComponentImporter = () => import('${splitUrl}')`,
)(),
])
}

prop.value = template.expression(
`lazyRouteComponent($$splitComponentImporter, 'component')`,
)()

programPath.pushContainer('body', [
template.statement(
`const $$splitComponentImporter = () => import('${splitUrl}')`,
`function DummyComponent() { return null }`,
)(),
])
}

prop.value = template.expression(
`lazyRouteComponent($$splitComponentImporter, 'component')`,
)()

programPath.pushContainer('body', [
template.statement(
`function DummyComponent() { return null }`,
)(),
])

found = true
found = true
}
} else if (prop.key.name === 'loader') {
const value = prop.value

let shouldSplit = true

if (t.isIdentifier(value)) {
existingLoaderImportPath =
getImportSpecifierAndPathFromLocalName(
programPath,
value.name,
).path

removeIdentifierLiteral(path, value)
}

// Prepend the import statement to the program along with the importer function
// exported identifiers should not be split
// since they are already being imported
// and need to be retained in the compiled file
const isExported = hasExport(ast, value)
shouldSplit = !isExported

if (!hasImportedOrDefinedIdentifier('lazyFn')) {
programPath.unshiftContainer('body', [
template.smart(
`import { lazyFn } from '@tanstack/react-router'`,
)() as t.Statement,
])
if (shouldSplit) {
removeIdentifierLiteral(path, value)
}
}

if (
!hasImportedOrDefinedIdentifier(
'$$splitLoaderImporter',
)
) {
programPath.unshiftContainer('body', [
template.statement(
`const $$splitLoaderImporter = () => import('${splitUrl}')`,
)(),
])
if (shouldSplit) {
// Prepend the import statement to the program along with the importer function
if (!hasImportedOrDefinedIdentifier('lazyFn')) {
programPath.unshiftContainer('body', [
template.smart(
`import { lazyFn } from '@tanstack/react-router'`,
)() as t.Statement,
])
}

if (
!hasImportedOrDefinedIdentifier(
'$$splitLoaderImporter',
)
) {
programPath.unshiftContainer('body', [
template.statement(
`const $$splitLoaderImporter = () => import('${splitUrl}')`,
)(),
])
}

prop.value = template.expression(
`lazyFn($$splitLoaderImporter, 'loader')`,
)()

found = true
}

prop.value = template.expression(
`lazyFn($$splitLoaderImporter, 'loader')`,
)()

found = true
}
}
}
Expand Down Expand Up @@ -252,6 +275,8 @@ export function compileCodeSplitVirtualRoute(opts: ParseAstOptions) {
)
}

const knownExportedIdents = new Set<string>()

babel.traverse(ast, {
Program: {
enter(programPath, programState) {
Expand Down Expand Up @@ -288,12 +313,31 @@ export function compileCodeSplitVirtualRoute(opts: ParseAstOptions) {
if (t.isObjectExpression(options)) {
options.properties.forEach((prop) => {
if (t.isObjectProperty(prop)) {
splitNodeTypes.forEach((type) => {
if (t.isIdentifier(prop.key)) {
if (prop.key.name === type) {
splitNodesByType[type] = prop.value
splitNodeTypes.forEach((splitType) => {
if (
!t.isIdentifier(prop.key) ||
prop.key.name !== splitType
) {
return
}

const value = prop.value

let isExported = false
if (t.isIdentifier(value)) {
isExported = hasExport(ast, value)
if (isExported) {
knownExportedIdents.add(value.name)
}
}

// If the node is exported, we need to remove
// the export from the split file
if (isExported && t.isIdentifier(value)) {
removeExports(ast, value)
} else {
splitNodesByType[splitType] = prop.value
}
})
}
})
Expand Down Expand Up @@ -449,6 +493,28 @@ export function compileCodeSplitVirtualRoute(opts: ParseAstOptions) {

deadCodeElimination(ast)

// if there are exported identifiers, then we need to add a warning
// to the file to let the user know that the exported identifiers
// will not in the split file but in the original file, therefore
// increasing the bundle size
if (knownExportedIdents.size > 0) {
const list = Array.from(knownExportedIdents).reduce((str, ident) => {
str += `\n- ${ident}`
return str
}, '')

const warningMessage = `These exports from "${opts.filename.replace('?' + splitPrefix, '')}" are not being code-split and will increase your bundle size: ${list}\nThese should either have their export statements removed or be imported from another file that is not a route.`
Copy link
Contributor

Choose a reason for hiding this comment

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

does this cause a problem with Start?
will Start properly work with some non-code-split routes?

it Start will not work, then we should throw a fatal error in case we are building for Start

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only capturing if the component or loader is being exported out of the route file. It'll detect it both for Start and Router since this happens during dev and at build time (not at runtime).

This is the example that are checking for:

// this export ident here
export const loadUser = async () => {
	return { user: 'john' }
}

export const Route = createFileRoute('/login')({
	loader: loadUser
})

In this case, we can't code-split the loadUser fn, since other external files may be imported from this source file.

So it needs to remain as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

understood, but is skipping a route from code splitting posing any problems in TanStack Start? i.e. could we just disable automatic code splitting for start and it would still work?

Copy link
Member Author

@SeanCassiere SeanCassiere Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

could we just disable automatic code splitting for start and it would still work?

We could choose to turn it off automatic code-splitting on our side, but currently, we force it ON in the Start config.

router/packages/start/src/config/index.ts

Lines 290 to 296 in 8fde2ef

TanStackRouterVite({
...tsrConfig,
autoCodeSplitting: true,
experimental: {
...tsrConfig.experimental,
},
}),

console.warn(warningMessage)

// append this warning to the file using a template
if (process.env.NODE_ENV !== 'production') {
const warningTemplate = template.statement(
`console.warn(${JSON.stringify(warningMessage)})`,
)()
ast.program.body.unshift(warningTemplate)
}
}

return generate(ast, {
sourceMaps: true,
minified: process.env.NODE_ENV === 'production',
Expand Down Expand Up @@ -517,3 +583,85 @@ function removeIdentifierLiteral(path: any, node: any) {
}
}
}

function hasExport(ast: t.File, node: t.Identifier): boolean {
let found = false

babel.traverse(ast, {
ExportNamedDeclaration(path) {
if (path.node.declaration) {
// declared as `const loaderFn = () => {}`
if (t.isVariableDeclaration(path.node.declaration)) {
path.node.declaration.declarations.forEach((decl) => {
if (t.isVariableDeclarator(decl)) {
if (t.isIdentifier(decl.id)) {
if (decl.id.name === node.name) {
found = true
}
}
}
})
}

// declared as `function loaderFn() {}`
if (t.isFunctionDeclaration(path.node.declaration)) {
if (t.isIdentifier(path.node.declaration.id)) {
if (path.node.declaration.id.name === node.name) {
found = true
}
}
}
}
},
ExportDefaultDeclaration(path) {
if (t.isIdentifier(path.node.declaration)) {
if (path.node.declaration.name === node.name) {
found = true
}
}
},
})

return found
}

function removeExports(ast: t.File, node: t.Identifier): boolean {
let removed = false

babel.traverse(ast, {
ExportNamedDeclaration(path) {
if (path.node.declaration) {
// declared as `const loaderFn = () => {}`
if (t.isVariableDeclaration(path.node.declaration)) {
path.node.declaration.declarations.forEach((decl) => {
if (t.isVariableDeclarator(decl)) {
if (t.isIdentifier(decl.id)) {
if (decl.id.name === node.name) {
path.remove()
removed = true
}
}
}
})
} else if (t.isFunctionDeclaration(path.node.declaration)) {
if (t.isIdentifier(path.node.declaration.id)) {
if (path.node.declaration.id.name === node.name) {
path.remove()
removed = true
}
}
}
}
},
ExportDefaultDeclaration(path) {
if (t.isIdentifier(path.node.declaration)) {
if (path.node.declaration.name === node.name) {
path.remove()
removed = true
}
}
},
})

return removed
}
27 changes: 27 additions & 0 deletions packages/router-plugin/tests/code-splitter/snapshots/retain-export-component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
const $$splitLoaderImporter = () => import('tsr-split:retain-export-component.tsx?tsr-split');
import { lazyFn } from '@tanstack/react-router';
import { createFileRoute, Outlet } from '@tanstack/react-router';
import { importedComponent as ImportedComponent } from '../shared/imported';
export function Layout() {
return <main>
<header style={{
height: HEADER_HEIGHT
}}>
<nav>
<ul>
<li>
<a href="/">Home</a>
</li>
</ul>
</nav>
</header>
<ImportedComponent />
<Outlet />
</main>;
}
export const Route = createFileRoute('/_layout')({
component: Layout,
loader: lazyFn($$splitLoaderImporter, 'loader')
});
const HEADER_HEIGHT = '63px';
export const SIDEBAR_WIDTH = '150px';
4 changes: 4 additions & 0 deletions packages/router-plugin/tests/code-splitter/snapshots/retain-export-component@split.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
console.warn("These exports from \"retain-export-component.tsx\" are not being code-split and will increase your bundle size: \n- Layout\nThese should either have their export statements removed or be imported from another file that is not a route.");
import { importedLoader } from '../shared/imported';
const loader = importedLoader;
export { loader };
Loading