Skip to content
This repository was archived by the owner on Dec 22, 2020. It is now read-only.

Conversation

@endiliey
Copy link

@endiliey endiliey commented May 11, 2019

This PR contains a:

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

Motivation / Use-Case

In some scenarios, a JS module may import a .css file and then webpack will process it with say, postcss-loader & css-loader.

If you need to nullify that .css file using this loader, the output of null-loader won't be valid CSS since CSS does not support single line comments // ... and thus, css-loader or postcss-loader will throw an error (SyntaxError) and webpack will stop bundling.

Test Plan

Using the newly added test file. We use css-loader to check if the transformed output is a valid css.

Before (using single-line)

expect(errors).toMatchInlineSnapshot(`
      Array [
        "./fixture.css
      Module build failed (from /mnt/c/Users/endij/Desktop/Linux/null-loader/node_modules/css-loader/dist/cjs.js):
      CssSyntaxError

      (1:1) Unknown word

      �[31m�[1m>�[22m�[39m�[90m 1 | �[39m// empty�[36m �[39m�[36m(null-loader)�[39m //
       �[90m   | �[39m�[31m�[1m^�[22m�[39m

       @ ./fixture2.js 1:0-35 3:15-21",
      ]
    `);

After (using multi-line)

expect(errors).toMatchInlineSnapshot(`Array []`);

Related PR

This is an attempt to continue #20. It seems that there is no sign of it's work will be continued by the author.

Close #20

@jsf-clabot
Copy link

jsf-clabot commented May 11, 2019

CLA assistant check
All committers have signed the CLA.

@endiliey endiliey force-pushed the multiline-comment branch from 2217116 to faee863 Compare May 11, 2019 07:48
@codecov
Copy link

codecov bot commented May 11, 2019

Codecov Report

Merging #21 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #21   +/-   ##
=======================================
  Coverage   77.77%   77.77%           
=======================================
  Files           2        2           
  Lines           9        9           
  Branches        1        1           
=======================================
  Hits            7        7           
  Misses          2        2
Impacted Files Coverage Δ
src/index.js 66.66% <50%> (ø) ⬆️

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 84998aa...faee863. Read the comment docs.

/***/ (function(module, exports) {
// empty (null-loader)
/* empty (null-loader) */
Copy link
Member

Choose a reason for hiding this comment

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

It is still js assets, so // also works. I think you need testing this in other way:

  1. Create css file
  2. Create custom url() loader (very simple implementation)
  3. Create custom css loader (very simple implementation)

Input:

a {
  background: url(path/to/something) no-repeat;
}

Output:

a {
  background: /* comment */ no-repeat;
}

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.

Wrong test

@endiliey endiliey mentioned this pull request May 15, 2019
6 tasks
@alexander-akait
Copy link
Member

Close in favor #24

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants