glromeo / esbuild-sass-plugin

esbuild plugin for sass
MIT License
155 stars 40 forks source link

sass minified files' sourcemap point to incorrect source file when using custom transform method. #185

Open mitcoding opened 2 months ago

mitcoding commented 2 months ago

The Key issue is that sourcemaps for bundled minified sass files only works as expected if no custom transform methods are called.

First there maybe more then one item that is causing the issue, so it's possible the issue is partially to blame in postcss and lightningcss postcss plugin.

The first potential issue I found is that even though I enabled sourcemaps in esbuild.build config options I also had to enable sourcemaps in esbuild-sass-plugin so that source maps could be passed to the transform method of said plugin, this was required because postcss requires passing { map: { prev: DECODED_PREV_SOURCE_MAP } } to the process method otherwise the sourcemap only shows the merged minified sass output file created by esbuild-sass-plugin, instead of the original source files. Secondly in order to even set { map: { prev: DECODED_PREV_SOURCE_MAP } } I had to write a custom function to parse out and decode the inlined source map included in the source attribute passed in the transform method (see function below (and in context in the attached source file for the build script build.txt:

const pathDelimiter        = __dirname.includes("\\") ? "\\" : "/";
const BUNDLE_ROOT          = ".."  + pathDelimiter + ".."  + pathDelimiter + ".."  + pathDelimiter + ".." + pathDelimiter + ".."  + pathDelimiter;
function getMap(source) {
    if (!enableSourceMaps) { return null; }

    const mapUrl = "/*# sourceMappingURL=data:application/json;charset=utf-8;base64,";
    const prevMap = Buffer
        .from(
            source.substring(source.indexOf(mapUrl) + mapUrl.length, source.length-3), 
            "base64"
        )
        .toString("utf-8")
        .replaceAll(/"sourceRoot":"[^"]+"/gi, '"sourceRoot":"' + BUNDLE_ROOT + BUNDLE_ROOT + '"')
    ;

    return { 
        prev: prevMap, 
        inline: true
    };
}

At one point it looks like esbuild-sass-plugin had merged in changes that passed "map" as the fourth attribute of transform not sure why this was removed and/or never merged in Expose sourcemaps to transform

The second issue is why I had to write .replaceAll(/"sourceRoot":"[^"]+"/gi, '"sourceRoot":"' + BUNDLE_ROOT + BUNDLE_ROOT + '"') in my custom sourcemap decoder. Without that line files listed in the generated source map have invalid file locations that look like:

resolve.path("__dirname", SOURCE_ROOT) + "/" + 
resolve.path("__dirname", SOURCE_ROOT) + "/" + 
resolve.path("__dirname", SOURCE_ROOT) + 
"/RELATIVE_ORIGIONAL_SOURCE_FILE_LOCATION"';

even if that string replace is removed the browser still ends up pointing to the wrong origional source files even if all the origional source files are listed as expected.

My package.json config to show that I'm using the latest versions of all the node dependencies

{
"scripts": {
    "dev": "node build.js --isDev && clientlib --verbose",
    "prod": "node build.js && clientlib --verbose",
    "aemfed": "aemfed -t \"http://admin:admin@localhost:4502\" -w \"../ui.apps/src/main/content/jcr_root/\""
  },
  "devDependencies": {
    "aem-clientlib-generator": "^1.8.0",
    "autoprefixer": "^10.4.19",
    "browserslist": "^4.23.0",
    "esbuild": "^0.23.0",
    "esbuild-plugin-browserslist": "^0.14.0",
    "esbuild-sass-plugin": "^3.3.1",
    "eslint": "^9.7.0",
    "eslint-plugin-react": "^7.35.0",
    "globals": "^15.8.0",
    "lightningcss": "^1.25.1",
    "postcss-lightningcss": "^1.0.0",
    "prop-types": "^15.8.1",
    "sass-embedded": "^1.77.8"
  },
  "dependencies": {
    "@fortawesome/fontawesome-pro": "^6.5.2",
    "@popperjs/core": "^2.11.8",
    "axios": "^1.6.8",
    "bootstrap": "^5.3.3",
    "react": "^18.2.0",
    "react-autosuggest": "^10.1.0",
    "react-dom": "^18.2.0",
    "react-modal": "^3.16.1",
    "smartystreets-javascript-sdk": "^5.1.1",
    "url-search-params-polyfill": "^8.2.5"
  },
  "browserslist": [
    "last 2 version",
    "> 1%"
  ]
}
mitcoding commented 2 months ago

Did further investigation today. One of the key issues is actually the minification step, and this bug exists in all three Esbuild, Postcss, & lighteningcss, so it compounds the issue in each step that minifies it. I know I said removing Postcss & Lightiningcss made the sourcemaps for css work, but that was only partially correct upon further investigation.

either Esbuild and/or Esbuild-sass-plugin will also on it's own even if the Transform Method is removed and minification is disabled mangle the sourceMap. It just not as obvious as the mangling is less because it just gets further mangled each time a minification step happens via a custom transform method.

With that said esbuild never generates a correct sourceMap even if minification is disabled, and there is no custom transform method used. However, you can get the correct sourcemap if the sourcemap is outputed to the filesystem during the transform step which means Esbuild and/or Esbuild-sass-plugin mangles it sometime after the transform method is called.

the Second issue is that postcss seems to screw up the sourceMap on it's own, by not including a nested sourcemap of esbuild's initial bundling step. To fix this issue I found that I needed to use lightiningcss directly, which gave the build script a nice performance boost.

However, when lightiningcss creates the Sourcemap it adds a nested sourcemap of esbuild's bundling step as required by the sourcemap specs, but Esbuild doesn't merge the nested sourcemap added by lightiningcss, which has the effect of doubling the size of the map file as it now includes all the source files' content and the bundled version's content of those source files' content.

I fixed this by including another library called sorcery. I think this is something Esbuild should include natively as it would help performance by not having to run a javascript nodejs lib like sorcery to do the merging, and is something that should happen only when using custom transform methods when maps are enabled.

build.txt -- updated build script that outputs the files before minification so it proves that it's esbuilds minifcation step that screws up source maps for css, and maybe js too I just don't know these sites' reactjs code in depth enough to tell if there is an issue or not with the source maps for js.

I should clarify when I said that the source maps are mengled what I mean is that the Dev Tools in Chrome/Firefox point to the wrong origional source file for a specific block of CSS that applies to the current HTML element being inspected. So the key issue is that the line and column location of the specific source code was recorded incorrectly in the sourcemap.

One thing I'm not sure on is if esbuild-sass-plugin is mangling the sourcemap after it calls the transform method or if it's esbuild that is causing the issue, I would assume the minification step is esbuild, but without diving into the source cdoe for esbuild-sass-plugin and esbuild I'm not sure the problem lies as esbuild won't even bundle the sass without the esbuild-sass-plugin.

glromeo commented 2 months ago

I had a hunch, from day one, that css minification perfection was a target unable to achieve due to some broken links in the chain (outside of my control) and that's why despite I did a best effort not to make things worse I didn't want to put any more thoughts into it. I appreciate your investigation and I reply mostly to say so. Should you get to the point of figuring out something that this plugin can actually do to improve the situation I will be happy to help... till then I leave this issue to simmer as a guide for others asking themselves the same question

mitcoding commented 2 months ago

@glromeo the interesting thing is that if we output the state of the css bundle and source map from the transform method before returning the results to esbuild-sass-plugin the sourcemap address look up is correct as far as I can tell. it just gets to mangled after the transform step. Which means you met your goal of not making it worse at least up to the transform step before the return is called.

which is why I attached a second version of my build script above. So that we can see that if we take the files from target/preMinified all three versions of the bundled css have correct sourcemaps (.origEsbuild = what is passed into the transform method from esbuild-sass-plugin, .merged is the state after running sorcery/lightingcss and sans .merged is the same output as .merged only it includes the nested bundle result from esbuild-sass-plugin in the map).

mitcoding commented 2 months ago

@glromeo Further findings. Tried jumping into both lightiningcss, esbuild-sass-plugin and esbuild source code. and I can't seem to find how they are determining to print rules in non-minified files. As far as I can tell esbuild-sass-plugin is just taking the output from sass-dart and in my specific config sass-embeded. It generates a sourcemap that is based on some selectors being on a single line and others all on their own invidual line.

e.g. some like: .foo, .bar { ... } others like: .more, less { .... }

Lightingcss always modifies the selectors so they are always consistantly on a single line and updates the sourcemap to match the new row and col locations of the selectors.

I also confirmed that using sass and lightiningcss directly via the CLI returns the same css and map output as your plugin returns just before the return call in the custom transform method.

I'm assuming, as I couldn't find where it goes after the return statement in the custom transformer call, that Esbuild then changes all the selectors to consistently be on their own line along with some other white space changes

e.g. .foo, .bar {} changes to:

.foo, bar { .... }

but never updates the sourceMap with the new Row and Col locations afer those whitespace changes and that is why the sourceMap's get broken.

Do you happen to know off the top of your head, after the transform method call returns its results, where it goes to next? As with that knowledge I could open a ticket with Esbuild to have them not modify any white space without also updating the sourcemap with the col and row changes that causes.

mitcoding commented 2 months ago

As a temp fix so that I could use the transform directly out of lightingcss I used the type option in a hacky way to disable further processing of the css by esbuild, which allows me to finally have working css sourceMaps now using esbuild. I'm attaching my build script for those that are looking to fix the same issue as we wait for esbuild and lightiningcss (lightiningcss only breaks if you minify with sourceMaps) can fix their code.

tempFixBuildScript.txt

I don't know if my hack will work with esbuild watch mode as I don't use the feature. So I will leave that as an exercise to who the script might help to figure out if it will need to be modified to make it work with esbuilds watch mode.