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

Generate Sass Sourcemaps #79

Merged
merged 39 commits into from
Aug 4, 2019
Merged

Conversation

Harald-LB
Copy link
Contributor

This PR solves Issue #71. External source maps are now generated by default along with the CSS file.

@DirtyF DirtyF requested a review from a team October 12, 2018 14:01
@DirtyF DirtyF added the feature label Oct 12, 2018
@jekyllbot jekyllbot closed this Dec 11, 2018
@pathawks

This comment has been minimized.

@Harald-LB

This comment has been minimized.

@pathawks pathawks changed the base branch from sassc to master December 12, 2018 17:09
@pathawks pathawks reopened this Dec 12, 2018
@pathawks
Copy link
Member

This PR was errantly closed because it was based on the sassc branch, which was deleted.
I have changed the base branch to master and reopened the PR. We may need some Git-Fu to get it all synced back up properly.

@sdumetz

This comment has been minimized.

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

Requesting a few edits here and there..

lib/fake-sass.rb Outdated Show resolved Hide resolved
lib/jekyll/converters/scss.rb Outdated Show resolved Hide resolved
spec/scss_converter_spec.rb Outdated Show resolved Hide resolved
lib/jekyll/converters/scss.rb Outdated Show resolved Hide resolved
lib/jekyll/converters/scss.rb Show resolved Hide resolved
@ashmaroli
Copy link
Member

@DirtyF Your site should build fine now. The bug has been fixed.

@ashmaroli
Copy link
Member

@Harald-LB Can you please add a small documentation under the heading Usage in the README for this project?
The section just needs to inform the user regarding the availability of sourcemaps and if they're able to configure the same in any way..

After this feature has been shipped, we'll update the docs for Jekyll Core accordingly.

@ashmaroli
Copy link
Member

Thank you for the documentation @Harald-LB.
Based on that documentation, I'd like to point to some issues with this PR:

  • precision and line_comments will be ignored in safe-mode
  • source_map_contents defaults to true but cannot be overridden by the user.

The above two scenarios are untested and undocumented.

@Harald-LB
Copy link
Contributor Author

@ashmaroli

precision and line_comments will be ignored in safe-mode

Can somebody explain to me what safe-mode ought to be?

source_map_contents defaults to true but cannot be overridden by the user.

I just tested, and yes ... you are right.

May be we should not set source_map_contents in line 168 of sass_configs. Or invert jekyll_sass_configuration, overrides in the call to Jekyll::Utils.deep_merge_hashes (I guess this
is what I wanted to achieve).

Or as a third alternative, just not document this option, like a number of other obscure options I did not mention (for example silent: when set, sassc will throw away all output).

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

sourcemaps is generated at first build, when I edit scss in devtools and save it, then an error pops:

[2019-02-21 18:45:50] ERROR `/assets/css/main.css.map' not found.

@ashmaroli
Copy link
Member

ashmaroli commented Feb 21, 2019

Can somebody explain to me what safe-mode ought to be?

@Harald-LB Safe mode is that mode where chances of arbitrary code execution are minimal-to-none.
The exact principle is beyond me, and what matters here is that GitHub Pages run in such an environment. All Jekyll builds on GHP are always in the --safe mode.

The comment I made earlier was based on the current implementation:

if safe?
  overrides
...

So, yes, it is best that those two options aren't exposed in the documentation now.

If you really want source_map_contents to be configurable, then you can define a new private helper method that in turn uses jekyll_sass_configuration to determine the final value returned just like how the :style is determined for example.

Harald-LB and others added 3 commits February 22, 2019 13:29
…ways true and is not user selectable. "precision" is now an undocumented option. "line_comments" can now be set in "save mode" too.
@Harald-LB
Copy link
Contributor Author

@DirtyF

sourcemaps is generated at first build, when I edit scss in devtools and save it, then an error pops:

I cannot reproduce this problem. Can you give me some more information

@Harald-LB
Copy link
Contributor Author

@ashmaroli

Source map generation cannot be disabled as of now.

Sounds a bit frustrated.

Should I add an option no_sourcemap? It's not a big deal.

@ashmaroli
Copy link
Member

Should I add an option no_sourcemap? It's not a big deal.

@Harald-LB If a simple false could disable source maps generation, that would be most welcome. Otherwise, no_sourcemap could do as well..
Thank you for all the work you've put into this so far.

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

Couple of tests for recently added methods required.

lib/jekyll/converters/scss.rb Show resolved Hide resolved
@DirtyF DirtyF added this to the v2.0 milestone Mar 24, 2019
@DirtyF

This comment has been minimized.

@DirtyF
Copy link
Member

DirtyF commented Aug 4, 2019

@jekyllbot: merge + minor

@jekyllbot jekyllbot merged commit f29e132 into jekyll:master Aug 4, 2019
jekyllbot added a commit that referenced this pull request Aug 4, 2019
@jekyll jekyll locked and limited conversation to collaborators Aug 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants