Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

fix(vite): fix distDir resolution #6215

Merged
merged 4 commits into from
Jul 29, 2022
Merged

fix(vite): fix distDir resolution #6215

merged 4 commits into from
Jul 29, 2022

Conversation

antfu
Copy link
Member

@antfu antfu commented Jul 29, 2022

πŸ”— Linked issue

close nuxt/nuxt#14446

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

The cause of nuxt/nuxt#14446 is that the dirs.ts module being bundled into chunks/index.js changes its relative path. Move it to utils to make it consistent across dev and build.

Also taking the chance to improve the code splitting by dynamically importing for optional plugins. Previously vite-node and rollup-plugin-visualizer are always imported regardless if the feature is enabled or not.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@netlify
Copy link

netlify bot commented Jul 29, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit daca6b6
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/62e3d1adb42b600008800ce7

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

❀️

import { fileURLToPath } from 'node:url'
import { resolve } from 'pathe'

export const distDir = resolve(fileURLToPath(import.meta.url), '../..')
Copy link
Member

Choose a reason for hiding this comment

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

This file was intentionally in top-level to match source and generated chunk path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but unbuild always extracts it into a chunk even when I have directly imported it in index.ts. Or maybe we need another top-level export dirs to make sure it's always on top-level?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a non-problem because we used to have only one file for this package.

Copy link
Member

Choose a reason for hiding this comment

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

I see. The only issue is that is approach can be easily broken if chunk generation changes with unbuild>rollup.

Currently, we use this workaround for the main nuxt pkg:

if (_distDir.endsWith('chunks')) { _distDir = dirname(_distDir) }

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, updated

@antfu antfu added bug Something isn't working ❗ p4-important Priority 4: bugs that violate documented behavior, or significantly impact perf labels Jul 29, 2022
@danielroe danielroe changed the title fix(vite): improve code spliting fix(vite): improve code splitting Jul 29, 2022
@pi0 pi0 changed the title fix(vite): improve code splitting fix(vite): fix distDir resolution Jul 29, 2022
@@ -58,7 +56,9 @@ export async function buildClient (ctx: ViteBuildContext) {
rootDir: ctx.nuxt.options.rootDir,
buildAssetsURL: joinURL(ctx.nuxt.options.app.baseURL, ctx.nuxt.options.app.buildAssetsDir)
}),
viteNodePlugin(ctx)
ctx.nuxt.options.experimental.viteNode
? await import('./vite-node').then(r => r.viteNodePlugin(ctx))
Copy link
Member

Choose a reason for hiding this comment

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

Since this is going to be the default option, I think we can safely import it normally when removed experimental flag. Difference is small but rollup this way can optimize startup time to add nested dependencies to top level dist/index.mjs

@pi0 pi0 merged commit 6b20d9e into main Jul 29, 2022
@pi0 pi0 deleted the fix/vite-improve-code-splitting branch July 29, 2022 12:33
@pi0 pi0 mentioned this pull request Aug 5, 2022
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.x bug Something isn't working ❗ p4-important Priority 4: bugs that violate documented behavior, or significantly impact perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vite-node breaks with commit d15c472
3 participants