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

Support asyncronous functions in config.url and config.import. #1277

Merged
merged 2 commits into from
Mar 24, 2021
Merged

Support asyncronous functions in config.url and config.import. #1277

merged 2 commits into from
Mar 24, 2021

Conversation

DavidArchibald
Copy link
Contributor

This PR contains a:

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

Motivation / Use-Case

Fixes #1236.

Breaking Changes

None

Additional Info

My apologies for the issue lingering open. Apparently I must've cleared all my notifications and missed your response. I don't believe anything like the schema needs updating (async functions are instance of Function) so the change is pretty small.

@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #1277 (f23b110) into master (e194e6b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1277   +/-   ##
=======================================
  Coverage   99.46%   99.47%           
=======================================
  Files          11       11           
  Lines         753      762    +9     
  Branches      258      261    +3     
=======================================
+ Hits          749      758    +9     
  Misses          4        4           
Impacted Files Coverage Δ
src/plugins/postcss-import-parser.js 100.00% <100.00%> (ø)
src/plugins/postcss-url-parser.js 100.00% <100.00%> (ø)

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 e194e6b...f23b110. Read the comment docs.

@DavidArchibald
Copy link
Contributor Author

Ah, I thought I had run the validations beforehand but must've missed the lint (it can sometimes be tricky to know what automated checks will be run). Little embarrassing to need two commits for such a small change so I'd expect they should be smushed or combined with --force.

Looks like you have the no-await-in-loop rule enabled. Would it be appropriate to add a commit that ignores that with // eslint-disable-next-line no-await-next-line or should it be refactored inside of the tasks promise array below? Promises that are skipped would have to be filtered out of the below loop although that's not complex. I'm asking because it's ultimately stylistic and I don't want to push assuming one way or another. Being so small a lot of routine questions and review can overwhelm the actual work put into the code, haha.

Lint errors for reference:

[lint:js      ] /home/runner/work/css-loader/css-loader/src/plugins/postcss-import-parser.js
[lint:js      ]   161:34  error  Unexpected `await` inside a loop  no-await-in-loop
[lint:js      ] 
[lint:js      ] /home/runner/work/css-loader/css-loader/src/plugins/postcss-url-parser.js
[lint:js      ]   264:32  error  Unexpected `await` inside a loop  no-await-in-loop

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.

await inside for of, it is bad for performance, we need refactor code and use Promise.all

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.

Thanks, I will look at this in near future

@DavidArchibald
Copy link
Contributor Author

Sounds good, hope it all adheres now (fail the first commit's message format). As a side note, I struggled with CRLF vs LF differences in the test, I'm assuming because the test snapshots are formatted with Linux/Mac line endings. WSL didn't seem to help in this case either. I struggled with that for a while before deciding to just ignore test failures complaining about \r\n vs \n. Do you have ideas about how to fix that, perhaps a Jest configuration?

@alexander-akait alexander-akait merged commit c5062db into webpack-contrib:master Mar 24, 2021
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.

Support Asyncronous functions for config.url and config.import
2 participants