-
Notifications
You must be signed in to change notification settings - Fork 1
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
LPS-128362 - Enable dartSass build option for themes #843
Conversation
CI is automatically triggering the following test suites:
|
ci:stop |
❌ ci:test:sf - 0 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-128362 1 Failed Jobs:For more details click here.
|
Jenkins Build:test-portal-source-format#4610 |
Jenkins Build:test-portal-acceptance-pullrequest(master)#7802 |
Changes look good. I added the "on hold" label until the release is out and integrated, at which point we can test. |
Testing this as we speak |
Ok, so this seems to work other than it basically doubles compilation time for themes 🙈 I think we have no other option since node-sass is dead, but... 🤔 @wincent, thoughts? |
Previously discussed here. We always knew it was going to be slower, but hopefully not by too much relative to our overall build process, and hopefully not for ever. What were the concrete numbers you saw? But at the end of the day, our hand is forced here by the fact that node-sass is a dead-end. Look on the bright side: it's a step closer to seeing the end of support requests related to node-sass exploding whenever it sees a different version of Node. |
K, I rebased, fixed SF and force-pushed.
I was seeing below 20s for node-sass and above 30s for dart-sass. Funny part of this is we're already using
I don't want to even try and see how slow this gets with Seeing as this is what it is, going to take it out of draft and give forward a try 🤞 |
ci:forward |
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
Alas, I tried and it's as bad as I expected 😂 |
Jenkins Build:test-portal-acceptance-pullrequest(master)#7826 |
Let's try this one more time... |
ci:forward |
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
Skipping previously passed test suites: |
All required test suite(s) passed. |
Pull request has been successfully forwarded to brianchandotcom#99625 |
Jenkins Build:test-portal-acceptance-pullrequest(master)#186 |
@jbalsas @wincent @bryceosterhaus Maybe |
|
Although they still have the huge deprecation notice on the readme:
which they added as part of the pretty decisive-sounding PR #3133 (related issue: #3123). One of the maintainers may have had a change of heart, but I am still inclined to think fool me once, shame on... shame on you... but fool me you can't get fooled again. |
Haha 👍 @wincent |
This is not intended to be merged until @liferay/npm-scripts is merged with a greater version than v37.1.2
This PR is intended to be used with
liferay-theme-tasksv10.3.0
which is included in the next npm-scripts release.I am sending this now as a draft now in case I am on leave when npm-scripts is updated. Feel free to test and merge once npm-scripts is updated.