Skip to content

feat: add insertionTag option #377

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

Closed
wants to merge 1 commit into from

Conversation

itaylor
Copy link

@itaylor itaylor commented Apr 12, 2019

This PR contains a:

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

Motivation / Use-Case

There are numerous bug reports around the net about flashes of unstyled content (FOUC) when using style-loader with sourcemaps:
facebook/create-react-app#6399
webpack-contrib/css-loader#613
https://stackoverflow.com/questions/36453826/how-to-stop-fouc-when-using-css-loaded-by-webpack
webpack/webpack-dev-middleware#51

Ultimately, all of these are caused by two things:

  1. Style loader chooses <link> elements when using sourcemaps, and those <link> elements are evaluated async, so the HTML may be rendered before the CSS.
  2. Style loader doesn't support adding sourcemaps on <style> elements, so if you want sourcemaps, you end up with FOUCs.

This change allows you to explicitly specify that you want to use <style> elements or <link> elements, or singleton <style> elements. It also adds support for putting sourcemaps into <style> elements, which works great in Chrome.

Breaking Changes

None, existing behavior is preserved if you don't pass the insertionTag option.

Additional Info

Other stuff I noticed:

There were no tests for <link> element rendering. I added one, and had to mock a couple of missing methods on URL from JSDOM to do so.

Also, the documented sourceMap option didn't do anything. The only place sourceMaps were used was in the <link> element code and it was looking at the object passed in from the css-loader and not the options. I left that as-is to preserve backward compatibility, but when adding support for source maps to <style> tags, I made it work as per the docs, based on the sourceMap option flag. I'm not sure if that's the right thing to do or to remove the do-nothing sourceMap option from the docs entirely and just rely on the upstream loader having passed a sourcemap or not.

Another question that might be worth discussion, if 'singleton' is only there for old IE, and modern browsers support sourcemaps in <style> tags, a "legacy free" version of this loader might be nice. It could cut down on the size and complexity and just support <style> tags, necessitating less options and less code.

@jsf-clabot
Copy link

jsf-clabot commented Apr 12, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 12, 2019

Codecov Report

Merging #377 into master will decrease coverage by 0.33%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #377      +/-   ##
=========================================
- Coverage    7.38%   7.05%   -0.34%     
=========================================
  Files           6       6              
  Lines         298     312      +14     
  Branches       90      94       +4     
=========================================
  Hits           22      22              
- Misses        210     220      +10     
- Partials       66      70       +4
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 81b4d18...95669f3. Read the comment docs.

@itaylor
Copy link
Author

itaylor commented Apr 12, 2019

Hmmm... I think something is wrong with the codecov setup? It's showing no coverage for most of the tests...
https://codecov.io/gh/webpack-contrib/style-loader/tree/master/src

Circleci is doing the same thing.
https://circleci.com/gh/webpack-contrib/style-loader/580?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

In the package.json it looks like the coverage tools are configured to only apply codecoverage on the src folder, but the tests that use test/utils.js run against the compiled versions in the dist folder.

    "test:coverage": "jest --collectCoverageFrom='src/**/*.js' --coverage",

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.

We don't want new option new version only support insert: <Function> to allow developers write own complex logic

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.

Also you break logic for other, please open issue and describe you problem

@itaylor
Copy link
Author

itaylor commented Apr 12, 2019

@evilebottnawi, the existing insertion options that are documented: insertInto and insertAt deal with selecting an element to insert the styles into and the placement of the style within that element. This feature allows you to specify which type of tag to insert, which as far as I can see, you can't do with insertInto or insertAt.

Do you have specific cases that I broke the logic for? I was very careful to avoid making any backward incompatible changes here, the logic only changes if you pass the new insertionTag option. If there's somewhere that I missed and there is a backward-incompatible change, can you please tell me what it is so that I can fix it?

@alexander-akait
Copy link
Member

Sorry we don't want this PR, we want keep loader as small as possible and as fast as possible, better create own loader for this purpose, anyway thanks and sorry for delay

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.

3 participants