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

sourcemap sources field gets url encoded #133

Closed
sapphi-red opened this issue Apr 19, 2022 · 7 comments · Fixed by #134
Closed

sourcemap sources field gets url encoded #133

sapphi-red opened this issue Apr 19, 2022 · 7 comments · Fixed by #134
Assignees
Labels
bug Something isn't working

Comments

@sapphi-red
Copy link

When a file is in a directory with a name including @, the sources field in sourcemap gets url encoded.
This does not happen with dart-sass.

.
└── src
    ├── @dir
    │   └── bar.scss
    └── foo.scss
sass.render({
  file: path.resolve('src/foo.scss'),
  outFile: path.resolve('dist/foo.css'),
  sourceMap: true
})

sass.render({
  file: 'src/foo.scss',
  outFile: 'dist/foo.css',
  sourceMap: true
})
[dart-sass] with resolve
{"version":3,"sourceRoot":"","sources":["../src/@dir/bar.scss","../src/foo.scss"],"names":[],"mappings":"AAAA;EACE;;;ACCF;EACE","file":"foo.css"}

-----------
[sass-embedded] with resolve
{"version":3,"sourceRoot":"","sources":["../src/%40dir/bar.scss","../src/foo.scss"],"names":[],"mappings":"AAAA;EACE;;;ACCF;EACE","file":"foo.css"}

-----------
[dart-sass] without resolve
{"version":3,"sourceRoot":"","sources":["file:///D:/documents/GitHub/sass-embedded-atmark-sourcemap-reproduction/src/@dir/bar.scss","file:///D:/documents/GitHub/sass-embedded-atmark-sourcemap-reproduction/src/foo.scss"],"names":[],"mappings":"AAAA;EACE;;;ACCF;EACE","file":"foo.css"}

-----------
[sass-embedded] without resolve
{"version":3,"sourceRoot":"","sources":["../src/%40dir/bar.scss","../src/foo.scss"],"names":[],"mappings":"AAAA;EACE;;;ACCF;EACE","file":"foo.css"}

Step to reproduce

  1. Clone https://github.com/sapphi-red/sass-embedded-atmark-sourcemap-reproduction.
  2. Run npm i.
  3. Run node sass.js
  4. See output.
@ntkme
Copy link
Contributor

ntkme commented Apr 19, 2022

The source map spec actually implies that sources are URLs:

If the sources are not absolute URLs after prepending of the “sourceRoot”, the sources are resolved relative to the SourceMap (like resolving script src in a html document).

@sapphi-red
Copy link
Author

sapphi-red commented Apr 19, 2022

It is valid to include @ in path of url. https://stackoverflow.com/questions/19509028/can-i-use-an-at-symbol-inside-urls

I added a directory named %40dir to reproduction.
Looks like dart-sass fails to handle this edge case.
%40dir/bar disappears from sources. I expect %2540dir/bar to be included if it is always url encoded.

[dart-sass] with resolve
{"version":3,"sourceRoot":"","sources":["../src/@dir/bar.scss","../src/foo.scss"],"names":[],"mappings":"AAAA;EACE;;;AADF;EACE;;;ACEF;EACE","file":"foo.css"}

-----------
[sass-embedded] with resolve
{"version":3,"sourceRoot":"","sources":["../src/%40dir/bar.scss","../src/foo.scss"],"names":[],"mappings":"AAAA;EACE;;;AADF;EACE;;;ACEF;EACE","file":"foo.css"}

@sapphi-red
Copy link
Author

Yes I expect sources field to be

  • ../src/@dir/bar.scss
  • ../src/%40dir/bar.scss
  • ../src/foo.scss

or

  • ../src/%40dir/bar.scss
  • ../src/%2540dir/bar.scss
  • ../src/foo.scss

.
The first one is all not url encoded. The second one is all url encoded.

@ntkme
Copy link
Contributor

ntkme commented Apr 20, 2022

As regarding the path after @import, I checked the FileSystemImporter, it turns out that it will call path.fromUri first, so that %40dir and @dir are actually both the same @dir, to import %40dir you will need to import %2540dir.

So this is an inconsistency between the output of the two.

@ntkme
Copy link
Contributor

ntkme commented Apr 20, 2022

The behavior difference is only shown in the legacy API, which for the embedded host it is just a wrapper of new API. I modified your test cases with the new compile API, and they all get the same result. So likely a bug in legacy API in embedded-host-node.

@ntkme
Copy link
Contributor

ntkme commented Apr 20, 2022

URL encode is done by this:

if (source.startsWith('file://')) {
return pathToUrlString(
p.relative(sourceMapDir, fileUrlToPathCrossPlatform(source))
);

@sapphi-red
Copy link
Author

%40dir and @dir are actually both the same @dir, to import %40dir you will need to import %2540dir.

Oh I was misunderstanding the behavior. Yes you were correct.
Now I think it does not need anything to be changed because there is a consistency. (But may need to be fixed if more strict compatibility is required.)

Also for without resolve ones, there is difference.

[dart-sass] without resolve
{"version":3,"sourceRoot":"","sources":["file:///D:/documents/GitHub/sass-embedded-atmark-sourcemap-reproduction/src/@dir/bar.scss","file:///D:/documents/GitHub/sass-embedded-atmark-sourcemap-reproduction/src/foo.scss"],"names":[],"mappings":"AAAA;EACE;;;ACCF;EACE","file":"foo.css"}

-----------
[sass-embedded] without resolve
{"version":3,"sourceRoot":"","sources":["../src/%40dir/bar.scss","../src/foo.scss"],"names":[],"mappings":"AAAA;EACE;;;ACCF;EACE","file":"foo.css"}

I will leave this issue open for the reasons above, but please feel free to close it.

@nex3 nex3 self-assigned this Apr 25, 2022
@nex3 nex3 added the bug Something isn't working label Apr 25, 2022
@nex3 nex3 closed this as completed in #134 Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants