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

Use the file option insetad of data for renderSync #46

Merged
merged 1 commit into from
Apr 18, 2018
Merged

Use the file option insetad of data for renderSync #46

merged 1 commit into from
Apr 18, 2018

Conversation

homer0
Copy link
Contributor

@homer0 homer0 commented Apr 17, 2018

What does this PR do?

As I mentioned on #45, when you use sourceMapEmbed, you can't the path to the original sources because the node-sass renderSync call was made using the data option, which is a string with the whole SASS, so the map source is set to stdin.

In order to get the real sources on a source map, you need to use the file option, instead of data, with the path of the file that will be processed.

Now, the reason renderSync is called with data is because the plugin allows you to send a data option with custom SASS code that will be prepended to the contents of the file.

Finally, the change I'm proposing is that, when you use the data option, the plugin will behave as always and call renderSync with the whole code; but if you don't use data, the plugin will call renderSync with the file option.

If you noticed that I changed a test asset file, I did it because its extension was .sass but the code was SCSS, and when you use file, you need to use the syntax that matches the extension of the file.

How should it be tested manually?

Use the plugin with the options.sourceMapEmbed option set to true, and don't include a data option.
The generated file will include a huge comment with the source map encoded on Base64; you can use base64decode.org to decode that it, and once you do it, you'll see that sources now points to the real files that were encoded.

And don't forget to npm test.

@homer0
Copy link
Contributor Author

homer0 commented Apr 17, 2018

@differui Not sure why did it fail on the Node 5 thread; I'll check it later and see if I can solve it.

@differui
Copy link
Collaborator

differui commented Apr 18, 2018

Thanks!
The CI problem exists in master branch too.I will update node versions to fix it.

@differui differui merged commit 2279325 into elycruz:master Apr 18, 2018
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.

None yet

2 participants