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

fix: inline style source maps #383

Merged

Conversation

jonathantneal
Copy link
Contributor

@jonathantneal jonathantneal commented Jul 19, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Currently, if source maps are enabled, style-loader uses Blob URLs within <link> tags, instead of <style> tags with inline CSS. These Blobs URLs were used to workaround a bug that preventing source maps in <style> tag from working in Chrome, but that bug was fixed in 2015. 🤔

Unfortunately, using <link> to load CSS is asynchronous, which causes the FOUC. These issues were resolved by #219, which was approved, merged, and then “lost”; mostly likely due to a mistake in branch management. 🤷‍♂

There have continued to be issues with the Blob URL implementation. As a result, Create React App has removed the ability to use CSS source maps in development. 😢

But now I have work to do. And that work involves CSS. And when I work with CSS, I want to use source maps. So I’ve done my research, discovered the preexisting work of others, and restored the fix for you in this PR. 🙏

If this is merged, the code will be shorter, faster, and peace source maps may be restored to the galaxy Create React App. 😂

Breaking Changes

Yes, the breaking change is that the convertToAbsoluteUrls option is removed because it is no longer needed. That removal might be the reason Codecov reports that this PR reduces coverage by 7.38%.

However, I would consider this an unbreaking change. 🤓

Additional Info

  1. This same fix was already approved once and only removed by mistake, while it was correctly added to vue-style-loader in 2016. That also means this is a stable fix. 💪
  1. In order for this PR to pass, I had to run npm audit --fix and then npm install --save-dev @commitlint/cli@8.1.0.

  2. Also, this PR incidentally fixes these issues related to Blob:

@jonathantneal jonathantneal changed the title Fix/style sourcemaps Fix Source Maps Jul 20, 2019
@jonathantneal jonathantneal changed the title Fix Source Maps Fix Inline Style Source Maps Jul 20, 2019
@jonathantneal jonathantneal changed the title Fix Inline Style Source Maps fix: inline style source maps Jul 20, 2019
This change is unrelated to this PR, but is required for CI to pass
@codecov
Copy link

codecov bot commented Jul 20, 2019

Codecov Report

Merging #383 into master will decrease coverage by 7.38%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #383      +/-   ##
========================================
- Coverage    7.38%     0%   -7.39%     
========================================
  Files           6      5       -1     
  Lines         298    244      -54     
  Branches       90     72      -18     
========================================
- Hits           22      0      -22     
+ Misses        210    184      -26     
+ Partials       66     60       -6
Impacted Files Coverage Δ
src/addStyles.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ddb01b...b981470. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Great job! Will be merged to next release, maybe we can add couple tests?

@do-adams
Copy link

do-adams commented Aug 7, 2019

Awesome work @jonathantneal

No more FOUC on development builds. That's really sweet, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants