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.

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

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

Merged
merged 11 commits into from
Sep 18, 2024

Conversation

SeanCassiere
Copy link
Member

@SeanCassiere SeanCassiere commented Sep 14, 2024

From https://discord.com/channels/719702312431386674/1283438765817331712

When using autoCodeSplitting, we need to take into account the scenario where the user is exporting their component or loader from the route file. Consider this route before code-splitting:

import * as React from 'react'
import { createFileRoute, Outlet } from '@tanstack/react-router'
import {
  importedComponent as ImportedComponent,
  importedLoader,
} from '../shared/imported'

export const loaderFn = () => {
  return importedLoader()
}

const 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: loaderFn,
})

const HEADER_HEIGHT = '63px'
export const SIDEBAR_WIDTH = '150px'
export const SIDEBAR_MINI_WIDTH = '80px'
const ASIDE_WIDTH = '250px'

export default Layout

An assumption can be made here, that if the user is exporting the component or loader from this file, then they expect to import it into another file.

As such, the current strategy of nuking the component or loader, once it has been detected for use in the createFileRoute function will not work, as a user wouldn't then be able to make the import from another file since it would no longer exist.

Therefore, if the component or loader is being exported, then code removal shouldn't happen and it should retain the item in the same compiled file. As such, since the item remains in the same file, we can skip performing an import.

A warning is also going to be added into the console, so the user knows that this function is not being code-split.

Copy link

nx-cloud bot commented Sep 14, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit cf3d8d1. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Sep 14, 2024

Open in Stackblitz

More templates

@tanstack/history

pnpm add https://pkg.pr.new/@tanstack/history@2334

@tanstack/create-router

pnpm add https://pkg.pr.new/@tanstack/create-router@2334

@tanstack/react-cross-context

pnpm add https://pkg.pr.new/@tanstack/react-cross-context@2334

@tanstack/react-router

pnpm add https://pkg.pr.new/@tanstack/react-router@2334

@tanstack/react-router-with-query

pnpm add https://pkg.pr.new/@tanstack/react-router-with-query@2334

@tanstack/router-arktype-adapter

pnpm add https://pkg.pr.new/@tanstack/router-arktype-adapter@2334

@tanstack/router-cli

pnpm add https://pkg.pr.new/@tanstack/router-cli@2334

@tanstack/router-devtools

pnpm add https://pkg.pr.new/@tanstack/router-devtools@2334

@tanstack/router-generator

pnpm add https://pkg.pr.new/@tanstack/router-generator@2334

@tanstack/router-plugin

pnpm add https://pkg.pr.new/@tanstack/router-plugin@2334

@tanstack/router-valibot-adapter

pnpm add https://pkg.pr.new/@tanstack/router-valibot-adapter@2334

@tanstack/router-vite-plugin

pnpm add https://pkg.pr.new/@tanstack/router-vite-plugin@2334

@tanstack/router-zod-adapter

pnpm add https://pkg.pr.new/@tanstack/router-zod-adapter@2334

@tanstack/start

pnpm add https://pkg.pr.new/@tanstack/start@2334

@tanstack/start-vite-plugin

pnpm add https://pkg.pr.new/@tanstack/start-vite-plugin@2334

@tanstack/virtual-file-routes

pnpm add https://pkg.pr.new/@tanstack/virtual-file-routes@2334

commit: cf3d8d1

@SeanCassiere SeanCassiere marked this pull request as ready for review September 14, 2024 01:12
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

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.

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

@schiller-manuel schiller-manuel merged commit df67018 into main Sep 18, 2024
5 checks passed
@schiller-manuel schiller-manuel deleted the codesplitting-retain-default-exports branch September 18, 2024 22:52
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.

2 participants