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

Custom file reader api #671

Merged
merged 9 commits into from Oct 29, 2019
Merged

Custom file reader api #671

merged 9 commits into from Oct 29, 2019

Conversation

@jquense
Copy link
Contributor

@jquense jquense commented Sep 19, 2019

not ready, this is some on going work likely needed for a better webpack loader integration, work in progress here: https://github.com/jquense/css-module-loader

@jquense
Copy link
Contributor Author

@jquense jquense commented Sep 19, 2019

cc @evilebottnawi

Copy link
Owner

@tivac tivac left a comment

I like where this is heading!

packages/processor/processor.js Outdated Show resolved Hide resolved
packages/processor/processor.js Show resolved Hide resolved
@alexander-akait
Copy link

@alexander-akait alexander-akait commented Sep 23, 2019

Yes, idea is good, better use promisify'd api as recommended above

@tivac
Copy link
Owner

@tivac tivac commented Sep 27, 2019

@jquense are you still planning to pursue this? I have some time and could tackle it if necessary since I think it's a big improvement over my original bad "always read from the filesystem" version.

@jquense
Copy link
Contributor Author

@jquense jquense commented Sep 27, 2019

@tivac I am still planning on it but need to get some stuff at work done first, so If you want to jump please feel free!

@jquense
Copy link
Contributor Author

@jquense jquense commented Oct 20, 2019

@tivac I'll pick this back up this week if you've not made additional progress on it.

packages/processor/processor.js Outdated Show resolved Hide resolved
@tivac
Copy link
Owner

@tivac tivac commented Oct 20, 2019

@jquense I have made zero progress on this, so glad to see you back at it! I'm on vacation atm so it'll be a little bit before I can provide proper reviews and replies but I'll try to not let it sit for too long.

It wasn't waiting for the file to actually be processed before returning in cases where the file was already being walked. This led to race conditions when trying to load two files at once. Fixing the race condition allows removing the file add cache.
@tivac tivac changed the title WIP: Custom file reader api Custom file reader api Oct 29, 2019
@tivac tivac marked this pull request as ready for review Oct 29, 2019
@codecov
Copy link

@codecov codecov bot commented Oct 29, 2019

Codecov Report

No coverage uploaded for pull request base (master@2c5a51d). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #671   +/-   ##
=========================================
  Coverage          ?   98.44%           
=========================================
  Files             ?       45           
  Lines             ?     1158           
  Branches          ?      179           
=========================================
  Hits              ?     1140           
  Misses            ?       15           
  Partials          ?        3
Impacted Files Coverage Δ
packages/processor/processor.js 100% <100%> (ø)

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 2c5a51d...d7aa72c. Read the comment docs.

@tivac tivac merged commit f1865c9 into tivac:master Oct 29, 2019
6 checks passed
@tivac
Copy link
Owner

@tivac tivac commented Oct 29, 2019

Thanks @jquense for all your work on this!

@jquense
Copy link
Contributor Author

@jquense jquense commented Oct 29, 2019

awesome 🙌 once this is published i'll put out a super beta of my webpack loader and run it through some paces on our apps. Lets see what breaks when we swap out css-modules

@jquense jquense deleted the custom-reader branch Oct 29, 2019
@alexander-akait
Copy link

@alexander-akait alexander-akait commented Oct 29, 2019

@jquense please ping me when it was ready, will be great to drop css-modules from css-loader in next release in favor your loader

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

Successfully merging this pull request may close these issues.

None yet

3 participants