Compile assets with dart-sass and esbuild

[n-studio]

Fixes #2091

[philipithomas]

Exciting! Is there a way to lint whether assets have been re-compiled?

[n-studio]

Exciting! Is there a way to lint whether assets have been re-compiled?

Not sure how to do that... but maybe I can trigger the assets build each time rake is called?

[n-studio]

Does someone know how to silence hound? This is annoying...

[pablobm]

@n-studio - Thank you for this. Just dropping a quick line to let you know that this is being looked into.

The first thing that I have noticed is that the generated assets break my test environment. For example: if I run the specs locally (Rails 6.1 at the moment), line 46 below breaks because the CSS is not loaded.

administrate/spec/features/edit_page_spec.rb

Lines 39 to 48 in be688bc

	it "displays selectable strings as dropdowns", :js do
	customer = create(:customer, kind: :standard)
	visit edit_admin_customer_path(customer)
	select "vip", from: "Kind"
	click_on "Update Customer"
	expect(page).to have_text("KIND")
	expect(page).to have_text("vip")
	end

As far as I can tell, this is because the generated assets gain precedence over the ones that Rails 6.1 would load, but there's some issue loading them and the browser only gets a 404, thus not getting a stylesheet.

I haven't had time to dig deeper than that for now. If you or other people interested in the PR could look into it, that would be great.

[n-studio]

@pablobm When I run the specs with Rails 6.1 locally all my specs pass green. Maybe you haven't compiled the assets before running the tests? If you run rake spec assets won't be compiled before running the tests, you have to either run rake or rake compile_assets; rake spec. Or did I miss something?

[pablobm]

Thank you for this. It looks promising and it may help us solve some other issues we have with JS. Some quick comments:

• My previous comment was my mistake, apologies. I tried your branch, then went back to main, and specs started failing because the previously generated assets were conflicting with the current setup.
• I think that the date picker is not working now. Some JS appears to load, and spec/features/orders_form_spec.rb:106 passes by manipulating the datepicker JS directly. I wonder if it's a CSS problem?
• Would you be able to rebase this? There have been a few PRs merged lately.
• Meanwhile, I'm going to try ask other people about this, to make sure I'm not missing anything.

[pablobm]

Is this line necessary? It scares me a bit that it sets a global in a bit of a hidden place.

[n-studio]

@pablobm This line is necessary for the tests to run because we use things like:

# spec/features/orders_form_spec.rb
page.execute_script(<<-JS)
        var date = moment("#{time_string}", "YYYY-MM-DD hh:mm:ss");
        $('[data-type="datetime"]').data("DateTimePicker").date(date);
      JS

I don't know how to import jQuery within a execute_script.
Maybe I could have capybara to click on the calendar instead of injecting JS.
Or I could use a very custom namespace such as window.administrate_jQuery = $;

[n-studio]

@pablobm The date picker is broken indeed, let me fix it. Btw can you help me silence Hound? I'm not familiar with it and I couldn't find how to exclude generated assets from being linted.

[n-studio]

I fixed the date picker and rebased here: n-studio#2
I will merge in this branch when I figure out how to silence hound.

[danielmorrison]

Does someone know how to silence hound? This is annoying...

Add an exclude to .scss-lint.yml for the compiled files.

[n-studio]

@danielmorrison I added:

scss_files: "lib/assets/stylesheets/**/*.scss"

exclude:
   - "lib/assets/stylesheets/reset/**"
   - "app/assets/builds/**/*"

but it didn't work...

[pablobm]

I think I figured out the issue with Hound. I merged a fix (hopefully) at #2138. Try rebasing and let's see if it works?

[pablobm]

Forgot to mention: after rebasing, you'll have to add an ignoreFiles entry to .stylelintrc.json. See https://stylelint.io/user-guide/configure/#ignorefiles

[n-studio]

@pablobm The Hound fix didn't seem to work 😖

To fix date-time-picker I had to copy the css from https://github.com/c4lliope/datetime_picker_rails
I couldn't figure out where the css was in npm repositories, my guess is that the css in the gem has been customized.

[n-studio]

If #2136 is accepted I can also drop date-time-picker completely.

[pablobm]

On the contrary, the change worked perfectly 🙂 The warnings about that generated CSS file are now gone, and what we have left is different and worth looking into.

• Some shoud be trivial, like the line length ones.
• The spaced-comment one is from eslint. I would add an exception to that specific file
• The ones about import being a reserved word... I think it's eslint too, and it can be configured to understand it: https://programmerah.com/solved-parsing-error-the-keyword-import-is-reserved-35034/
• The function-calc-no-invalid ones look like it's misinterpreting the hyphens in the variable name as arithmetic "minus" signs. Looking up the issue, it looks like it might be possible to fix it: https://github.com/stylelint-scss/stylelint-scss/blob/master/src/rules/dollar-variable-pattern/README.md
• Etc.

Would you be able to sort those?

[n-studio]

@pablobm Yes, sorry I've been busy, I try to fix that asap. What do you think about #2136 ?

[pablobm]

@n-studio - No rush. Re: #2136, there's a question open on compatibility, but I think we'll resolve it soon. Perhaps do as if it's happening for now, and don't worry too much about the date picker.

[n-studio]

@pablobm I fixed the lint! Do you need me to squash and rebase?

[nickcharlton]

Thank you for opening this and the effort you've taken to keep it up to date so far. I've been thinking about this PR over the last few days and I'm thinking that there's a completely different approach to take: remove Sass entirely.

This PR makes quite a lot of changes in one go, which I'm not so keen on doing. Introducing dart-sass like this needs us to change how we're handling JavaScript dependencies at the same time. There's been a lot of churn over the last few years in how Rails asset handling has been recommended, and whilst we do need to do a lot of work here, I'm keen to go slowly on it and ride out the change until it settles down.

A long-running issue we've been having is out of date dependencies because they've come from a time when we'd install them from Ruby Gems. It's worthwhile first removing or updating how we do those before making big changes to the way we handle assets, as our potential solution then might be very different. I become aware of dartsass-rails today, too.

For Sass itself, we're not using many of the features which it supports and a quick review suggests that leaning on CSS variables will be good enough to replace those we already use, and hard coding colours where we've calculated the values.

I've roped in @dcyoung-dev to try out the Sass replacement, which I'd like to see the results of (plus us removing other out of date dependencies) before making any decisions here.

[n-studio]

Hi @nickcharlton, thank you for your reply!

I'm using my own fork for my own projects so I'm not in a hurry for a new release, but I think other people might be interested in having a release compatible with Rails 7 as soon as possible.
I would recommend to use my PR first and then remove or update the dependencies one by one and update the css to avoid sass completely, which seems to be a time consuming task.

[brunoprietog]

I agree, this pr would allow me to use administrate in my project. It could be taken as a temporary solution while you remove Sass.

[pablobm]

Closing in favour of #2311