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

Allow function in watchOptions.ignored #197

Merged
merged 3 commits into from Nov 24, 2021

Conversation

markjm
Copy link
Contributor

@markjm markjm commented Jun 14, 2021

The globs are turned into regex's anyway, so seems like a pretty simple change. Have a webpack discussion thread on the topic as well where I suggest aligning watchOptions.ignored and WatchIgnorePlugin

@markjm
Copy link
Contributor Author

markjm commented Jun 14, 2021

Related to discussion post here webpack/webpack#13566

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #197 (b308647) into main (c651129) will decrease coverage by 0.87%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   92.67%   91.80%   -0.88%     
==========================================
  Files           6        6              
  Lines        1038     1037       -1     
  Branches      245      245              
==========================================
- Hits          962      952      -10     
- Misses         76       85       +9     
Flag Coverage Δ
integration 91.80% <100.00%> (-0.88%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/DirectoryWatcher.js 91.45% <100.00%> (-1.55%) ⬇️
lib/watchpack.js 93.85% <100.00%> (-0.84%) ⬇️

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 c651129...b308647. Read the comment docs.

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.

Can you add test case?

@markjm
Copy link
Contributor Author

markjm commented Jun 14, 2021

@alexander-akait done - thanks for the suggestion, because I found an issue with the test I wrote. Tests +1 / Hubris +0

@markjm
Copy link
Contributor Author

markjm commented Jun 22, 2021

@alexander-akait ive patched this into our project and have been using it. Also had an unexpected performance improvement because I was able to tailor the regexs we use to be more performant than the globtoregex regexs :)

lib/watchpack.js Outdated Show resolved Hide resolved
@markjm
Copy link
Contributor Author

markjm commented Nov 11, 2021

@sokra / @alexander-akait - I have reworked these changes to simply allow a custom test function instead of trying to mess with provided regex's and flags. Now you can simply provide a function to ignore, which I can document on webpack side as well.

@alexander-akait
Copy link
Member

CI is broken

@markjm markjm changed the title Suggestion: allow regex's in watchOptions.ignored Allow function in watchOptions.ignored Nov 12, 2021
@markjm markjm closed this Nov 12, 2021
@markjm markjm reopened this Nov 12, 2021
@markjm
Copy link
Contributor Author

markjm commented Nov 12, 2021

@alexander-akait i think the test may be flaky because (1) the test is working in various polling/non-polling matrix entries, (2) #203 (just some other arbitrary PR) sees the same test failing in random stages as well, and (3) the failure does not repro for me locally.

Please let me know if I am doing something wrong here

move path normalization to option normalization
@sokra sokra closed this Nov 24, 2021
@sokra sokra reopened this Nov 24, 2021
@sokra sokra merged commit e71b62b into webpack:main Nov 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.

None yet

3 participants