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

Do not generate source map in Jekyll::Converters::Scss#convert #159

Open
UlyssesZh opened this issue Nov 20, 2023 · 3 comments
Open

Do not generate source map in Jekyll::Converters::Scss#convert #159

UlyssesZh opened this issue Nov 20, 2023 · 3 comments

Comments

@UlyssesZh
Copy link

generate_source_map_page(source_map)

Inside Jekyll::Converters::Scss#convert, the method generate_source_map_page is called, where a source map page is generated and pushed to site.pages. This is not a good approach. All pages should have been generated in the generation phase of the Jekyll site, not in the conversion phase. A better approach would be to create a class inheriting Jekyll::Generator dedicated to generate this source map.

I found this problem because it made the source map not generated when I try to implement multithread rendering feature for Jekyll (jekyll/jekyll#9485).

@ntkme
Copy link
Contributor

ntkme commented Nov 21, 2023

It’s a major change the rendering order, and if we force sass to be compiled eagerly, you lost the chance of optimizing performance by compiling sass in parallel with other pages. sass-embedded uses a long lived external process and it can take advantage of concurrent compilations up to 15 threads per compiler process on 64-bit systems. However, note that today the converter is single-threaded for simplicity.

In my opinion it’s better to treat the css and source map outputs as static file assets instead of treating them as part of site.pages, this way sass can be compiled in parallel with other pages but it probably requires some major refactoring on both jekyll and jekyll-sass-converter.

@UlyssesZh
Copy link
Author

If we force sass to be compiled eagerly, you lost the chance of optimizing performance by compiling sass in parallel with other pages.

Actually, we don't have to compile Sass eagerly because Jekyll do not require a Page to have its final contents available during the generation phase. The contents can be rendered later in the conversion phase. We can demand the source map page to be converted later than the CSS page, when the contents of the source map are actually available, which is possible once Jekyll supports specifying conversion priority for pages (jekyll/jekyll#9485 (comment)).

Since Jekyll's conversion phase is single-threaded for now, pages added later are guaranteed to be converted later. A workaround like this may be a start:

  • Define a page dedicated for CSS, and attr_accessor :source_map_contents.
  • Define a generator dedicated for Sass. Generate a CSS page as well as a source map page (the source map page must be added later than the CSS page). Store a reference to the CSS page in the source map page (source_map_page.css_page).
  • In convert of the Sass converter, populate css_page.source_map_contents.
  • Define a converter dedicated for source map. In convert, simply return page.css_page.source_map_contents.

@ntkme
Copy link
Contributor

ntkme commented Nov 21, 2023

While your proposal would work, adding more complexity get around current design limitations is not good for long term maintenance of a big community project like Jekyll. Having a better design to support concurrency would be preferred over adding more and more code to make it work under the current design.

In other words, while it is nice to have multi-threading support in Jekyll 4.x, in my opinion there will be too many hacks that would be difficult to maintain as a community project. It would be better to put this on 5.x roadmap and do a redesign there.

Ideally all kinds of raw inputs should be able to be compiled concurrently, one input should be able to generate more than one outputs, and pages containing collection references can be detected and blocked for compilation until references are solved.

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

No branches or pull requests

2 participants