Error in preprocessor sourcemapping: "Cannot read property 'length' of undefined"

[benmccann]

Describe the bug
The new preprocessor sourcemapping feature seems to sometimes cause errors (CC @halfnelson @milahu)

Logs

> [email protected] build /Users/babichjacob/Repositories/sapper-postcss-template
> cross-env NODE_ENV=production sapper build --legacy

> Building...

[!] (plugin svelte) TypeError: Cannot read property 'length' of undefined
/Users/babichjacob/Repositories/sapper-postcss-template/src/routes/index.svelte
TypeError: Cannot read property 'length' of undefined
    at sourcemap_add_offset (/Users/babichjacob/Repositories/sapper-postcss-template/node_modules/svelte/compiler.js:22087:23)
    at get_replacement (/Users/babichjacob/Repositories/sapper-postcss-template/node_modules/svelte/compiler.js:28575:10)
    at /Users/babichjacob/Repositories/sapper-postcss-template/node_modules/svelte/compiler.js:28635:21
    at async Promise.all (index 0)
    at async replace_async (/Users/babichjacob/Repositories/sapper-postcss-template/node_modules/svelte/compiler.js:28551:52)
    at async preprocess_tag_content (/Users/babichjacob/Repositories/sapper-postcss-template/node_modules/svelte/compiler.js:28618:22)
    at async preprocess (/Users/babichjacob/Repositories/sapper-postcss-template/node_modules/svelte/compiler.js:28644:10)
    at async Object.transform (/Users/babichjacob/Repositories/sapper-postcss-template/node_modules/rollup-plugin-svelte/index.js:100:23)
    at async ModuleLoader.addModuleSource (/Users/babichjacob/Repositories/sapper-postcss-template/node_modules/rollup/dist/shared/rollup.js:18289:30)
    at async ModuleLoader.fetchModule (/Users/babichjacob/Repositories/sapper-postcss-template/node_modules/rollup/dist/shared/rollup.js:18345:9

To Reproduce
git clone [email protected]:babichjacob/sapper-postcss-template.git

Upgrade to svelte 3.30 and rollup-plugin-svelte 7 following the upgrade guide (example of that here: sveltejs/sapper-template#289)

sourcemap = false -> no error
sourcemap = "inline" -> that error
sourcemap = true -> that error

Severity
Several people have reported this issue on Discord. It seems to happen relatively frequently

[dimfeld]

My analysis so far:
The PostCSS processor in svelte-preprocess is returning a SourceMapGenerator instead of whatever the svelte compiler is expecting it to return. This SourceMapGenerator is coming directly from the postcss package. I'm not sure where exactly the SourceMapGenerator is supposed to turn into an actual source map, but that seems to be the missing step AFAICT.

Update:
Looks like just changing the postcss transformer isn't sufficient.

This is certainly not the exact right fix, but adding this in the Svelte compiler.js at line 28636, just before the call to get_replacement allows everything to build without errors:

if (processed.map?._mappings) {
    processed.map = processed.map.toString();
}

[jacobdalamb]

omg thank you

[jacobdalamb]

My analysis so far:
The PostCSS processor in svelte-preprocess is returning a SourceMapGenerator instead of whatever the svelte compiler is expecting it to return. This SourceMapGenerator is coming directly from the postcss package. I'm not sure where exactly the SourceMapGenerator is supposed to turn into an actual source map, but that seems to be the missing step AFAICT.

Update:
Looks like just changing the postcss transformer isn't sufficient.

This is certainly not the exact right fix, but adding this in the Svelte compiler.js at line 28636, just before the call to get_replacement allows everything to build without errors:

if (processed.map?._mappings) {
    processed.map = processed.map.toString();
}

it's giving me UnhandledPromiseRejectionWarning: /Users/jt/dev/code-challenges-sapper/node_modules/svelte/compiler.js:28636 if (processed.map?._mappings) { SyntaxError: Unexpected token '.'

[dimfeld]

Then your Node version is too old to support that syntax. You're probably better off just using Svelte 3.29 for now.

[nickreese]

Phew, glad I'm not alone. Also seems to be originating from svelte-preprocess.

[jacobdalamb]

Then your Node version is too old to support that syntax. You're probably better off just using Svelte 3.29 for now.

I'm using Svelte 3.17.3 ......

[dimfeld]

If you're seeing this bug, you're using Svelte 3.30.

https://bytearcher.com/articles/semver-explained-why-theres-a-caret-in-my-package-json/

[benmccann]

It was explained: https://bytearcher.com/articles/semver-explained-why-theres-a-caret-in-my-package-json/

That package.json specifies ^3.17.3 which means get the latest 3.x release, which is 3.30. The actual version used is specified in the package-lock.json. Try grep -r -A2 svelte package-lock.json

The method this bug refers to did not exist until 3.30, which means you're either using 3.30 or encountering a different issue.

[jacobdalamb]

It was explained: https://bytearcher.com/articles/semver-explained-why-theres-a-caret-in-my-package-json/

That package.json specifies ^3.17.3 which means get the latest 3.x release, which is 3.30. The actual version used is specified in the package-lock.json. Try grep -r -A2 svelte package-lock.json

The method this bug refers to did not exist until 3.30, which means you're either using 3.30 or encountering a different issue.

Damn I didn't known it did that, thank you and sorry for being arrogant :/
It worked with the ’~‘, thank you

[milahu]

thanks for the bug ; )

The PostCSS processor in svelte-preprocess is returning a SourceMapGenerator instead of whatever the svelte compiler is expecting it to return. This SourceMapGenerator is coming directly from the postcss package.

postcss is using this one
https://github.com/mozilla/source-map/blob/master/lib/source-map-generator.js

problem is, this one cannot produce "decoded" mappings
as the library remapping can consume

This is certainly not the exact right fix, but adding this in the Svelte compiler.js at line 28636, just before the call to get_replacement allows everything to build without errors:

if (processed.map?._mappings) {
   processed.map = processed.map.toString();
}

yes, that will fix the problem, at the cost of a slightly slower compile
(unnessesary encode + decode steps)
a little less bad is

processed.map = processed.map.toJSON();

the fastest solution is to fork the SourceMapGenerator._serializeMappings() method
and make it produce "decoded" mappings
(and ideally push that feature to upstream ....)

// not tested but "im feeling lucky"

import { compareByGeneratedPositionsInflated } from 'source-map/lib/util';

if (processed.map.constructor.name == 'SourceMapGenerator') {

  /**
   * patch function _serializeMappings()
   * to return mappings in "decoded" format
   */
  processed.map._serializeMappings = function () {
    let previousGeneratedLine = 1;
    let result = [[]];
    let resultLine;
    let resultSegment;
    let mapping;

    // optimized
    const sourceIdx = this._sources.toArray().reduce((acc, val, idx) => (acc[val] = idx, acc), {});
    const nameIdx = this._names.toArray().reduce((acc, val, idx) => (acc[val] = idx, acc), {});

    const mappings = this._mappings.toArray();
    resultLine = result[0];

    for (let i = 0, len = mappings.length; i < len; i++) {
      mapping = mappings[i];

      if (mapping.generatedLine > previousGeneratedLine) {
        while (mapping.generatedLine > previousGeneratedLine) {
          result.push([]);
          previousGeneratedLine++;
        }
        resultLine = result[mapping.generatedLine - 1]; // line is one-based
      } else if (i > 0) {
        if (
          !compareByGeneratedPositionsInflated(mapping, mappings[i - 1])
        ) {
          continue;
        }
      }

      resultLine.push([mapping.generatedColumn]);
      resultSegment = resultLine[resultLine.length - 1];

      if (mapping.source != null) {
        resultSegment.push(...[
          //this._sources.indexOf(mapping.source),
          sourceIdx[mapping.source], // optimized
          mapping.originalLine - 1, // line is one-based
          mapping.originalColumn
        ]);
        if (mapping.name != null) {
          resultSegment.push(
            //this._names.indexOf(mapping.name)
            nameIdx[mapping.name] // optimized
          );
        }
      }
    }
    return result;
  }

  // call the patched function to get a "decoded" sourcemap
  processed.map = processed.map.toJSON();
}

now who said monkey-patching is bad? ; )

edit: optimized indexOf to cache-object
edit: bugfix for line numbers
edit: add .toArray()

[halfnelson]

I'm guessing the correct fix here is probably to detect malformed source maps from preprocessors and log an appropriate error for the user.

[tanhauhau]

agree with @halfnelson, where svelte should handle malformed sourcemaps better with an appropriate error,

and an actual fix the sourcemap would have to come from the upstream, postcss svelte preprocessor

[dummdidumm]

While this is true, I think we should also be pragmatic and handle this specific case because I don't think upstream will just change this , others may even rely on this specific behavior.

[milahu]

"malformed" .. these are not malformed, but in a different "decoded" format, which is still better than getting encoded mappings (these must be decoded for remapping, which is sloooow, why there should be a warning when svelte receives encoded mappings)

we already accept one "decoded" format (the "dense array" format that remapping supports), why not support another one? its not like there are millions of formats that we all must support .. i thought svelte people care about performance? (or when did the trolls take over?)

[benmccann]

Is there no standard decoded format?

Are you saying that changing SourceMapGenerator._serializeMappings() upstream would produce the same decoded format we already accept or a new one?

[dummdidumm]

If it's from source-map I doubt you'll get it merged upstream, it wasn't updated in over a year, the project is kind of "finished" it seems.

[milahu]

Is there no standard decoded format?

no. the spec only defines the encoded format

the "dense array" format, as produced by magic-string (etc)
and consumed by remapping (etc) is the result of
decoding the VLQ values directly into arrays
for example AAAA becomes [0, 0, 0, 0]
only relative indices are converted to absolute indices.
this format is also used by sourcemap-codec (decode, encode)

SourceMapGenerator has a different format
which is simply one long array of mapping objects
line-numbers are one-based, etc

these formats are both not ideal for remapping
but still better than the encoded format

changing SourceMapGenerator._serializeMappings() upstream

no, that function is needed to produce the encoded format
a new method could be added to produce the dense-array-format

but then, that format is not specified, and only a de-facto-standard
cos some tools use it, with slight variation
for example, the version property is missing sometimes
so ..
i would say, its the consumer's job to have an import function

monkey-patching the _serializeMappings (see my 2nd last post)
is just one quick-n-dirty way to do this

if monkey-patching is too dirty, we could fork the toJSON method

function SourceMapGenerator_serializeMappings() {
  // see my 2nd last post
}

function SourceMapGenerator_toJSON() {
  const map = {
    version: this._version,
    sources: this._sources.toArray(),
    names: this._names.toArray(),

    // this is the only diff
    //mappings: this._serializeMappings()
    mappings: SourceMapGenerator_serializeMappings.apply(this)

  };
  if (this._file != null) {
    map.file = this._file;
  }

  // these are probably not needed
  if (this._sourceRoot != null) {
    map.sourceRoot = this._sourceRoot;
  }
  if (this._sourcesContents) {
    map.sourcesContent = this._generateSourcesContent(
      map.sources,
      map.sourceRoot
    );
  }

  return map;
}


if (processed.map.constructor.name == 'SourceMapGenerator') {
  processed.map = SourceMapGenerator_toJSON.apply(processed.map)
}

[benmccann]

Could we use your _serializeMappings within Svelte as a new method which takes a SourceMapGenerator instead of monkey patching?

[milahu]

yes, of course

i would keep the toJSON.apply(map) syntax
to keep toJSON and _serializeMappings similar to their original functions

the test could be changed to

if (
  processed.map._mappings &&
  processed.map.constructor.name == 'SourceMapGenerator'
) {
  // import decoded mappings from mozilla/source-map

[flayks]

Same issue here

[ParkYoungWoong]

Solved with npm i -D [email protected]

[Conduitry]

This should be fixed in 3.30.1 - we now support source maps created by SourceMapGenerator from Mozilla's source-map library. Thanks @milahu!