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

Support ingestion of inline, data-uri sourcemaps in input .css #600

Closed

Conversation

cspotcode
Copy link
Contributor

This patch enables clean-css to ingest sourcemaps within input .css files where the sourcemap is an inline data URI.

jakubpawlowicz and others added 3 commits June 2, 2015 20:15
Rebasing import statements without url had a nasty edge case where
a subsequent content ended up wrapped into a `url()`.
@jakubpawlowicz
Copy link
Collaborator

@cspotcode Thanks for the PR - I'll adapt your code a bit and merge it manually.

It would be nice to have a testcase too - it can be sth as simple as https:/jakubpawlowicz/clean-css/blob/master/test/source-map-test.js#L428 with source map as Base64 https:/jakubpawlowicz/clean-css/blob/master/test/fixtures/source-maps/styles.css.map

if(isDataUri) {
// source map's path is the same as the source file it comes from
sourceMapPath = path.resolve(self.options.root, sourceFile);
var buffer = dataUriToBuffer(sourceMapFile);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just use new Buffer(sourceMapFile, 'base64').toString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need the data URI parser to strip off the data URI's mimetype and grab
its charset declaration. Data URIs, surprisingly, default to ASCII
encoding, so it's important to respect the data URI's charset when parsing
it. See also: stylus/stylus#1935

On Sun, Jun 14, 2015 at 11:15 AM, Jakub Pawlowicz [email protected]
wrote:

In lib/utils/input-source-map-tracker.js
#600 (comment)
:

   if (isRemote) {
     return fetchMapFile(self, sourceMapFile, context, proceedToNext);
   } else {
     var sourceFile = context.files[context.files.length - 1];
  •    var sourceDir = sourceFile ? path.dirname(sourceFile) : self.options.relativeTo;
    

- var sourceMapPath = path.resolve(self.options.root, path.join(sourceDir || '', sourceMapFile));

  •    var sourceMapData = fs.readFileSync(sourceMapPath, 'utf-8');
    
  •    self.trackLoaded(sourceFile || undefined, sourceMapPath, sourceMapData);
    
  •    var sourceMapPath, sourceMapData;
    
  •    if(isDataUri) {
    
  •      // source map's path is the same as the source file it comes from
    
  •      sourceMapPath = path.resolve(self.options.root, sourceFile);
    
  •      var buffer = dataUriToBuffer(sourceMapFile);
    

Can't we just use new Buffer(sourceMapFile, 'base64').toString()?


Reply to this email directly or view it on GitHub
https:/jakubpawlowicz/clean-css/pull/600/files#r32380660.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we can do without those 2 dependencies given what they do: https:/TooTallNate/node-data-uri-to-buffer/blob/master/index.js#L44

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's your decision, depending on your philosophy about dependencies. Given
that they're small, simple libraries, I don't see a practical downside to
using them.

You'll also need to handle data URIs that are not base64 encoded.

On Sun, Jun 14, 2015 at 12:37 PM, Jakub Pawlowicz [email protected]
wrote:

In lib/utils/input-source-map-tracker.js
#600 (comment)
:

   if (isRemote) {
     return fetchMapFile(self, sourceMapFile, context, proceedToNext);
   } else {
     var sourceFile = context.files[context.files.length - 1];
  •    var sourceDir = sourceFile ? path.dirname(sourceFile) : self.options.relativeTo;
    

- var sourceMapPath = path.resolve(self.options.root, path.join(sourceDir || '', sourceMapFile));

  •    var sourceMapData = fs.readFileSync(sourceMapPath, 'utf-8');
    
  •    self.trackLoaded(sourceFile || undefined, sourceMapPath, sourceMapData);
    
  •    var sourceMapPath, sourceMapData;
    
  •    if(isDataUri) {
    
  •      // source map's path is the same as the source file it comes from
    
  •      sourceMapPath = path.resolve(self.options.root, sourceFile);
    
  •      var buffer = dataUriToBuffer(sourceMapFile);
    

I'm pretty sure we can do without those 2 dependencies given what they do:
https:/TooTallNate/node-data-uri-to-buffer/blob/master/index.js#L44


Reply to this email directly or view it on GitHub
https:/jakubpawlowicz/clean-css/pull/600/files#r32381526.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to keep dependencies to the minimum. I made some small changes to your code and pushed it as #604 - let me know what you think.

…'m using my patched clean-css in other projects.
@jakubpawlowicz
Copy link
Collaborator

Superseded by #604.

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.

2 participants