Repository navigation
feat: add CSS source map support - #6969
Elements2720 wants to merge 5 commits into
Conversation
stephan-dum
left a comment
There was a problem hiding this comment.
thank you so much for implementing it that fast <3
i added some suggestions and feedback
already looking forward to use it!
| @@ -0,0 +1,56 @@ | |||
| jest.mock( | |||
| '../../sys/node/autoprefixer.js', | |||
There was a problem hiding this comment.
i was already wondering when reading the source code inside src/compiler/optimize/autoprefixer.ts
this import is non existing for me.
const getProcessor = (autoprefixerOpts: d.AutoprefixerOptions): CssProcessor => {
const { postcss, autoprefixer } = require('../sys/node/autoprefixer.js'); // <--- non existing
if (!cssProcessor) {
cssProcessor = postcss([autoprefixer(autoprefixerOpts)]);
}
return cssProcessor;
};it does not surface because src/compiler/optimize/optimize-css.ts checks for errors and returns the result as fallback
result = await autoprefixCss(inputOpts.input, inputOpts.autoprefixer ?? null);
if (hasError(result.diagnostics)) {
return result;
}There was a problem hiding this comment.
The module is generated during the compiler build: src/sys/node/bundles/autoprefixer.js is bundled as sys/node/autoprefixer.js. The virtual Jest mock is intentional because unit tests run before that build artifact exists. Added a test comment documenting this.
| return generator.toJSON() as d.SourceMap; | ||
| }; | ||
|
|
||
| const getLineAndColumn = (text: string, index: number) => { |
There was a problem hiding this comment.
calling this repetitive can be quite expensive, we could consider a binary search instead, maybe something into this direction: https://github.com/cenfun/monocart-locator/blob/c227b9574146ee0b6a6ee7355df2ac747b91b979/lib/line-parser.js#L72
There was a problem hiding this comment.
Fixed. Source-map line/column lookup now precomputes line starts and uses binary search instead of rescanning the prefix for every token.
| file: compileOpts.file, | ||
| style: compileOpts.style as any, | ||
| styleImportData: compileOpts.styleImportData as any, | ||
| sourceMapCss: compileOpts.sourceMap !== false, |
There was a problem hiding this comment.
do we need to check if sourceMap.css is set as well?
There was a problem hiding this comment.
This is the legacy TranspileOptions path, whose sourceMap option is boolean or inline; it does not use the validated build sourceMap.css object. sourceMapCss is therefore derived from compileOpts.sourceMap !== false here.
| return withoutSourceMap; | ||
| } | ||
| const sourceMaps = Array.isArray(sourceMap) ? sourceMap : [sourceMap]; | ||
| return `${withoutSourceMap}${sourceMaps.map((map) => `\n/*# sourceMappingURL=${map} */`).join('')}`; |
There was a problem hiding this comment.
applying multiple source maps to the same file is not ideal
There was a problem hiding this comment.
Fixed. HMR now selects the source map matching each style element, so it does not append multiple maps to one element.
| }; | ||
|
|
||
| const appendStyleSourceMap = (styleText: string, sourceMap?: string | string[]) => { | ||
| const withoutSourceMap = styleText.replace(/\s*\/\*[#@]\s*sourceMappingURL=[^*]*\*\/\s*$/i, ''); |
There was a problem hiding this comment.
i would prefer to remove any existing source map already a build time
There was a problem hiding this comment.
The HMR path strips any existing trailing source-map annotation before applying the replacement map. Runtime registration preserves an existing annotation only when no replacement map is supplied.
| if (!sourceMap) { | ||
| return css; | ||
| } | ||
| const sourceMapUrl = `data:application/json;charset=utf-8;base64,${Buffer.from(JSON.stringify(sourceMap)).toString('base64')}`; |
There was a problem hiding this comment.
i would prefer external source maps if possible as this will increase bundle size
| return results; | ||
| }; | ||
|
|
||
| const sourceMapToDataUrl = (sourceMap: d.SourceMap) => |
There was a problem hiding this comment.
it would be nice to have them as external source maps
There was a problem hiding this comment.
For dist-global-styles, implemented external .map emission and a relative CSS reference. CSS embedded in component JavaScript has no standalone CSS asset to reference, so those maps remain attached per style entry.
There was a problem hiding this comment.
For production bundles this is still not ideal as it will bloat the bundle size, the specs are marking this field as optional:
The file field is an optional name of the generated code that this source map is associated with. It's not specified if this can be an URL, relative path name, or just a base name. Source map generators may choose the appropriate interpretation for their contexts of use.
so it should be possible to place the file next to the js bundle (ie entryX.css.map) as long as the source map reference in the css output is correct this should not be a problem.
Alternatively we could also put them next to the collection css output but i am not sure how they are consumed and if the reference would break if processed further.
| // do this BEFORE transformations on css files | ||
| if (shouldParseCssDocs && cmp != null) { | ||
| cmp.styleDocs = cmp.styleDocs || []; | ||
| const cssParseResults = await parseCssImports( |
There was a problem hiding this comment.
i would expect that this is also included into the source map
| ...getDependencySubset(cssParseResults.imports, transformResults.dependencies), | ||
| ); | ||
| } else { | ||
| const cssParseResults = await parseCssImports( |
There was a problem hiding this comment.
i would expect that this is also included in the source map
| // concat all css @imports into one file | ||
| // when the entry file is a .css file (not .scss) | ||
| // do this BEFORE transformations on css files | ||
| const cssParseResults = await parseCssImports(config, compilerCtx, buildCtx, id, id, transformResults.code); |
There was a problem hiding this comment.
i would expect that this is also included in the source map
| // the output but only updated it to use url() instead. Let's go ahead and concat | ||
| // the url() css files into one file like we did for raw .css files. Do this | ||
| // AFTER transformations on non-css files | ||
| const cssParseResults = await parseCssImports( |
There was a problem hiding this comment.
i would expect that this is also included in the source map,
i also wonder why not do this only at the end?
| @@ -185,7 +189,7 @@ export const initializeComponent = async ( | |||
| style = expandPartSelectors(style); | |||
There was a problem hiding this comment.
i guess that will change the style and our mapping is destroyed 🤔
i wonder if its possible to do that already at build time and generate a source map for it
| .join('')}`; | ||
| }; | ||
|
|
||
| const createStyleSourceMapIndex = (sourceMaps: d.StyleSourceMap[]) => { |
There was a problem hiding this comment.
this will only work with inline source maps, can't we use individual styles for each styleUrl entry?
i am asking because we really want the source map also in our production build so that consumers of our component library see whats going on to ease reporting issues back to us
There was a problem hiding this comment.
Implemented separate runtime entries for each styleUrl, with matching per-entry source maps. This avoids relying on a single concatenated inline map in production component builds.
|
@Elements2720 let me know if you need any support, i could write an e2e test for example if you like |
|
@stephan-dum check the changes, i have noted your review on the code. |
|
|
||
| if (isString(input.tag) && input.encapsulation === 'scoped') { | ||
| const scopeId = getScopeId(input.tag, input.mode); | ||
| results.styleText = scopeCss(results.styleText, scopeId, false); |
There was a problem hiding this comment.
this will also alter the style and needs to be included in the source map
There was a problem hiding this comment.
Fixed. The pre-scope CSS is now used as the source-map input, and the scoped output is mapped back through the plugin map before optimization. Added a regression test for scoped source-map composition.
| results.styleText = scopeCss(results.styleText, scopeId, false); | ||
| } | ||
|
|
||
| const cssImports = getCssToEsmImports(varNames, results.styleText, input.file, input.mode); |
There was a problem hiding this comment.
i am a bit confused here:
in plugin.ts we are calling parseCssImports, so there should not be any imports anymore
is this a left over for the legacy transpile function?
There was a problem hiding this comment.
The production bundle path already resolves CSS imports in runPluginTransformsEsmImports. This parser remains as the fallback for the public transpile API, which still receives raw CSS and needs to generate CSS module imports.
| const createStyleSourceMapExpression = (input: d.TransformCssToEsmInput, results: d.TransformCssToEsmOutput) => { | ||
| const parentSourceMap = JSON.stringify(results.styleSourceMap) ?? 'undefined'; | ||
| if (input.sourceMap && results.imports.length > 0) { | ||
| return `[].concat(${results.imports.map((cssImport) => `${cssImport.varName}SourceMap`).join(', ')}, ${parentSourceMap})`; |
There was a problem hiding this comment.
not sure if this will work: as mentioned before this functionality is just used by transpile which is only used in test code, this will never be passed back to to loaders and even then if could not resolve because its lacks the ?tag query parameter
There was a problem hiding this comment.
Removed the imported source-map expression. CSS imports are legacy transpile behavior here; production component CSS imports are resolved before this transform, and the transpile path now preserves its existing concatenated-style behavior with one map for the transformed CSS.
| return css; | ||
| } | ||
| const sourceMapUrl = sourceMapToDataUrl(sourceMap); | ||
| return `${css}\n/*# sourceMappingURL=${sourceMapUrl} */`; |
There was a problem hiding this comment.
this is still an inline source map it would be nice to write it to disc and just use the reference
There was a problem hiding this comment.
Implemented for file-based global styles. dist-global-styles now writes a relative sourceMappingURL reference in the CSS and emits the matching .map file beside it. Component styles embedded in JS remain per-style entries because there is no standalone CSS asset to reference.
|
|
||
| addNativeStaticStyle(classMembers, component, buildCtx); | ||
|
|
||
| expect(classMembers).toHaveLength(2); |
There was a problem hiding this comment.
this assertion could be more precise and descriptive
There was a problem hiding this comment.
Fixed the assertion to verify both generated accessors and their names: style first, sourceMap second.
| cmp.styles.forEach((style) => { | ||
| if (Array.isArray(style.externalStyles) && style.externalStyles.length > 0) { | ||
| if (typeof style.styleIdentifier !== 'string') { | ||
| createStyleIdentifier(cmp, style, { separateStyles: true }); |
There was a problem hiding this comment.
not sure if this can even happen because while generating the style this is already called which is a prerequisite for this function to run
this can produce side effects as style.styleIdentifier is altered each time the function is called and the return value is also not used
There was a problem hiding this comment.
Removed the fallback call and its side effect. Source-map identifiers are now consumed only after style generation has initialized them.
| const firstNonImportIndex = tsSourceFile.statements.findIndex((statement) => !ts.isImportDeclaration(statement)); | ||
| const importStatements = | ||
| firstNonImportIndex === -1 ? tsSourceFile.statements : tsSourceFile.statements.slice(0, firstNonImportIndex); | ||
| const nonImportStatements = firstNonImportIndex === -1 ? [] : tsSourceFile.statements.slice(firstNonImportIndex); |
There was a problem hiding this comment.
a comment would be nice to explain why the first non import statement it the cutoff point
There was a problem hiding this comment.
Added a comment explaining that the first non-import statement is the cutoff so generated source-map declarations stay after imports and do not execute before their style bindings.
|
|
||
| const flattenStyleText = (cssText: StyleText): string[] => { | ||
| if (Array.isArray(cssText)) { | ||
| return cssText.reduce<string[]>((styles, text) => styles.concat(flattenStyleText(text)), []); |
There was a problem hiding this comment.
Kept the recursive flattening intentionally because the runtime still supports optional ES5 builds, where Array.prototype.flat is not guaranteed. Added a comment documenting that compatibility choice.
There was a problem hiding this comment.
i would expect the bundler to handle the target, i you need to think about every es5 feature now spread and so on will also not work
| const currentStyleSheets = styleSheets.map((styleSheet) => { | ||
| let stylesheet: CSSStyleSheet; | ||
| if (styleSheet.constructor === currentWindow.CSSStyleSheet) { | ||
| stylesheet = styleSheet; |
There was a problem hiding this comment.
Renamed the source and destination stylesheet variables to sourceStyleSheet and currentStyleSheet.
|
thanks for all the effort you put into this, already look great 🚀 the only missing points from my side are:
|
|
@Elements2720 i investigated a bit the scopeCss and expandPartSelectors problem as i think this is the main blocker at the moment: scopeCss is already executed at compile time but with As far as i understood it (please correct my if i am wrong):
as alternative as this code is only executed if shadowNeedsScopedCss we could also demand that developer need a browser that supports scope native and therefor doesn't need to execute this logic let me know if i can support you with something, ie the e2e test to verify everything maps back correctly. |
Summary
Fixes #6964
Verification
One unrelated existing full-suite failure remains in src/compiler/output-targets/dist-lazy/test/generate-lazy-module.spec.ts (direct-dependency sort expectation).