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

feat: upgrade sass-loader support to ^8 #736

Closed
wants to merge 3 commits into from

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Apr 19, 2020

Close #654

Breaking changes from sass-loader changelog:

@Lyrkan do you have an idea oh what to do with the failing test? Use DISABLE_UNSTABLE_CHECKS on it?

Thanks!

@Lyrkan
Copy link
Collaborator

Lyrkan commented Apr 22, 2020

@Lyrkan do you have an idea oh what to do with the failing test? Use DISABLE_UNSTABLE_CHECKS on it?

Weird fail... since it is on the "Lowest versions" job you can either:

  • find the dependency that causes it and bump the min version so it matches the one we have in yarn.lock
  • use DISABLE_UNSTABLE_CHECKS to bypass that check entirely

In my opinion it would probably be better to avoid DISABLE_UNSTABLE_CHECK if we can (especially if it's just a matter of updating something).

@weaverryan
Copy link
Member

@Kocal about the failure, interestingly enough, on #746 (Vue3 support) I see the same failure. But not in "lowest", in that PR, I see it on the normal build. It seems like Webpack may have tweaked something internally that is causing our order to change in entrypoints.json, but I'm not sure....

@Kocal
Copy link
Contributor Author

Kocal commented May 6, 2020

Hum, that's weird...

Does it mean that I don't have to fix this issue there, and we can ignore CI failures to review and merge this PR?

Because ATM, the only reason I keep this PR as draft is because of the CI failure 😅

@weaverryan
Copy link
Member

I found the issue. Just change the order in the test so that the test passes. In Webpack 4.22.0, a "deterministic ordering bug" was apparently fixed - https://github.com/webpack/webpack/releases/tag/v4.22.0 - I believe that caused Webpack to start outputting things in a different order. I don't think it's actually a problem - just something that's different before and after this version. Because this PR bumps Webpack higher than 4.22, it shouldn't be problem. Though... I'm puzzled as to why this wouldn't cause the tests to fail across all builds... since all builds now use higher than 4.22. So maybe I'm still missing a piece of the puzzle. I DO know that, on my Vue3 PR, I determined that Webpack 4.22.0 was the version that "toggled" the different 0.js and .js ordering.

@Kocal
Copy link
Contributor Author

Kocal commented May 7, 2020

PR rebased and conflicts fixed.

@stof
Copy link
Member

stof commented May 7, 2020

@weaverryan as it was not deterministic before, it is possible that it would fail only randomly...

@weaverryan weaverryan mentioned this pull request May 9, 2020
@weaverryan
Copy link
Member

Closing in favor of #758 - I got the weird test failure figured out over there :)

@weaverryan weaverryan closed this May 9, 2020
@Kocal Kocal deleted the feat/sass-loader-8 branch May 10, 2020 05:46
weaverryan added a commit that referenced this pull request May 10, 2020
This PR was merged into the master branch.

Discussion
----------

Feat/sass loader 8

Fixes weird test issue for #736 - I'm trying to see if I can debug the "lowest" test failure.

It appears the problem is with Webpack > 4.22 and Vue < 2.5.0.

Commits
-------

fb24ca7 Vue 2.5.0 as minimum
9911316 build(deps): upgrade webpack version to ^4.36.0
960481f feat: upgrade sass-loader support to ^8
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.

Support for sass-loader 8.0.0
4 participants