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: include matchResource in entry request #720

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jquense
Copy link

@jquense jquense commented Mar 16, 2021

Sorry @alexander-akait, I'm always creating a bunch of work for you for super niche features. I think this should be clear but happy to explain it more if you want.

This PR contains a:

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

Motivation / Use-Case

Breaking Changes

Additional Info

@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #720 (b92fbe2) into master (6ae4c3e) will decrease coverage by 19.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #720       +/-   ##
===========================================
- Coverage   89.67%   70.43%   -19.25%     
===========================================
  Files           6        6               
  Lines         775      778        +3     
  Branches      239      240        +1     
===========================================
- Hits          695      548      -147     
- Misses         77      212      +135     
- Partials        3       18       +15     
Impacted Files Coverage Δ
src/loader.js 82.31% <100.00%> (-10.05%) ⬇️
src/utils.js 53.84% <0.00%> (-34.62%) ⬇️
src/index.js 61.11% <0.00%> (-26.93%) ⬇️

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 6ae4c3e...b92fbe2. Read the comment docs.

@jquense
Copy link
Author

jquense commented Mar 16, 2021

arg, why is trying to upgrade a single dependency breaking everything!

@jquense
Copy link
Author

jquense commented Mar 16, 2021

codecov you don't make any sense.

@alexander-akait
Copy link
Member

hm, I don't think it is good idea to do in pitch phase, we can faced with infinity loop...

@jquense
Copy link
Author

jquense commented Mar 16, 2021

you'd certainly know better than me. It seemed ok to me since the original request included it. What's happening is that the original module (with matchResource) is sort of being ignored, since mini-extract and style-loader force a new module to be created, this time without the matchResource (since it's not in the request anymore)

I had also thought that the !!./loaders might prevent a loop since there is no resource matching for loaders?

@alexander-akait
Copy link
Member

I don't think we should touch matchResource in pitch mode here, I think you need do on own side using pitch, so I want to look how you do it right now

@jquense
Copy link
Author

jquense commented Mar 16, 2021

the simplest case is the base4-loader. which doesn't work with style-loader or mini-extract.

The way more complicate case is here, still a WIP.

https://github.com/4Catalyzer/astroturf/blob/f723dc2d23ece81417f34daf1d6562dba6f1fc15/src/inline-loader.ts

FYI this does seem to work if i manually set matchResource on the module, but that seems like a bad idea?

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.

None yet

2 participants