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: dev sourcemap #8269

Merged
merged 2 commits into from May 22, 2022
Merged

fix: dev sourcemap #8269

merged 2 commits into from May 22, 2022

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented May 22, 2022

Description

in dev mode, we don't need to generate sourcemap, if generate a sourcemap will point to a null file.
this PR will clean importGlob and dynamicImport generate sourcemap in dev.

Additional context

config.build had value when mode === 'build'


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev patak-dev merged commit 505f75e into vitejs:main May 22, 2022
@poyoho poyoho deleted the fix/dev-sourcemap branch May 22, 2022 07:05
@bluwy
Copy link
Member

bluwy commented May 22, 2022

In dev mode, if sourcemaps aren't needed should we force not generate them? Currently they are still generated based on the build config config.build.sourcemap.

@poyoho
Copy link
Member Author

poyoho commented May 22, 2022

in dev mode vite just make the default value in build option

export function resolveBuildOptions(raw?: BuildOptions): ResolvedBuildOptions {
const resolved: ResolvedBuildOptions = {
target: 'modules',
polyfillModulePreload: true,
outDir: 'dist',
assetsDir: 'assets',
assetsInlineLimit: 4096,
cssCodeSplit: !raw?.lib,
cssTarget: false,
sourcemap: false,
rollupOptions: {},
minify: raw?.ssr ? false : 'esbuild',
terserOptions: {},
write: true,
emptyOutDir: null,
manifest: false,
lib: false,
ssr: false,
ssrManifest: false,
reportCompressedSize: true,
chunkSizeWarningLimit: 500,
watch: null,
...raw,
commonjsOptions: {
include: [/node_modules/],
extensions: ['.js', '.cjs'],
...raw?.commonjsOptions
},
dynamicImportVarsOptions: {
warnOnError: true,
exclude: [/node_modules/],
...raw?.dynamicImportVarsOptions
}
}
// handle special build targets
if (resolved.target === 'modules') {
// Support browserslist
// "defaults and supports es6-module and supports es6-module-dynamic-import",
resolved.target = [
'es2020', // support import.meta.url
'edge88',
'firefox78',
'chrome87',
'safari13' // transpile nullish coalescing
]
} else if (resolved.target === 'esnext' && resolved.minify === 'terser') {
// esnext + terser: limit to es2021 so it can be minified by terser
resolved.target = 'es2021'
}
if (!resolved.cssTarget) {
resolved.cssTarget = resolved.target
}
// normalize false string into actual false
if ((resolved.minify as any) === 'false') {
resolved.minify = false
}
if (resolved.minify === true) {
resolved.minify = 'esbuild'
}
return resolved
}

and in build mode vite will load the user config.

const buildOutputOptions = (output: OutputOptions = {}): OutputOptions => {
return {
dir: outDir,
format: ssr ? 'cjs' : 'es',
exports: ssr ? 'named' : 'auto',
sourcemap: options.sourcemap,
name: libOptions ? libOptions.name : undefined,
generatedCode: 'es2015',
entryFileNames: ssr
? `[name].js`
: libOptions
? resolveLibFilename(libOptions, output.format || 'es', config.root)
: path.posix.join(options.assetsDir, `[name].[hash].js`),
chunkFileNames: libOptions
? `[name].[hash].js`
: path.posix.join(options.assetsDir, `[name].[hash].js`),
assetFileNames: libOptions
? `[name].[ext]`
: path.posix.join(options.assetsDir, `[name].[hash].[ext]`),
// #764 add `Symbol.toStringTag` when build es module into cjs chunk
// #1048 add `Symbol.toStringTag` for module default export
namespaceToStringTag: true,
inlineDynamicImports:
output.format === 'umd' ||
output.format === 'iife' ||
(ssr && typeof input === 'string'),
...output
}
}

@bluwy
Copy link
Member

bluwy commented May 22, 2022

in dev mode vite just make the default value in build option

The user could set build.sourcemap: true in dev mode though as they're sharing the same config in dev + build. Is it safe it that case too, or should we force set build.sourcemap: false in dev mode?

On the side, I'm not familiar with sourcemaps myself and it seems a bit weird to disable in dev mode 😬

@poyoho
Copy link
Member Author

poyoho commented May 22, 2022

yep, it safe. Because vite only load the user build option when mode === 'build'

@bluwy
Copy link
Member

bluwy commented May 22, 2022

I still don't quite understand though (and sorry for the noise), but if I put console.log(config.build.sourcemap) in the importAnalysis change in this PR. I get true if config.build.sourcemap is true in dev mode. IIUC you're implying that the console.log would be false.

And if I revert the change in this PR (except for the test), it still seem to pass. Maybe I'm missing something here but if you can show how to reproduce the issue, that we be really helpful for me to understand the issue.

@poyoho
Copy link
Member Author

poyoho commented May 22, 2022

oh sorry, I misunderstood. You are right, I think we should force set build.sourcemap: false in dev mode 👍.

Later I found out that the default value is overridden with a user parameter :p

export function resolveBuildOptions(raw?: BuildOptions): ResolvedBuildOptions {
const resolved: ResolvedBuildOptions = {
target: 'modules',
polyfillModulePreload: true,
outDir: 'dist',
assetsDir: 'assets',
assetsInlineLimit: 4096,
cssCodeSplit: !raw?.lib,
cssTarget: false,
sourcemap: false,
rollupOptions: {},
minify: raw?.ssr ? false : 'esbuild',
terserOptions: {},
write: true,
emptyOutDir: null,
manifest: false,
lib: false,
ssr: false,
ssrManifest: false,
reportCompressedSize: true,
chunkSizeWarningLimit: 500,
watch: null,
...raw,
commonjsOptions: {
include: [/node_modules/],
extensions: ['.js', '.cjs'],
...raw?.commonjsOptions
},
dynamicImportVarsOptions: {
warnOnError: true,
exclude: [/node_modules/],
...raw?.dynamicImportVarsOptions
}
}
// handle special build targets
if (resolved.target === 'modules') {
// Support browserslist
// "defaults and supports es6-module and supports es6-module-dynamic-import",
resolved.target = [
'es2020', // support import.meta.url
'edge88',
'firefox78',
'chrome87',
'safari13' // transpile nullish coalescing
]
} else if (resolved.target === 'esnext' && resolved.minify === 'terser') {
// esnext + terser: limit to es2021 so it can be minified by terser
resolved.target = 'es2021'
}
if (!resolved.cssTarget) {
resolved.cssTarget = resolved.target
}
// normalize false string into actual false
if ((resolved.minify as any) === 'false') {
resolved.minify = false
}
if (resolved.minify === true) {
resolved.minify = 'esbuild'
}
return resolved
}

you can setting config.build.sourcemap: true in worker the see the error

@BenceSzalai
Copy link
Contributor

Just a note, that a sourcemap is required when one would like to debug the script in their IDE, since the imports and some other parts of the code is manipulated by vite before serving preventing the IDE to properly match it to the source files. See #7767 for details. Apparently it affects JetBrains IDE's as well as VisualCode and probably others as well.

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.

None yet

4 participants