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: [2.5.1] mark getCssModule and getCssDependency as public #900

Closed
wants to merge 2 commits into from

Conversation

barak007
Copy link
Contributor

@barak007 barak007 commented Jan 17, 2022

This PR contains a:

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

Motivation / Use-Case

Mark getCssModule and getCssDependency as public
getCssModule is the base of our integration to mini-css-extract-plugin added support done here #703
We ended up not using getCssDependency but I feel like it should also be public.
These are the basic building blocks for proper integration to css bundling in webpack

Breaking Changes

Fix types

Additional Info

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 17, 2022

CLA Not Signed

@barak007 barak007 changed the title fix: mark getCssModule and getCssDependency as public fix: [2.5.1] mark getCssModule and getCssDependency as public Jan 17, 2022
@alexander-akait
Copy link
Member

There is ts bug here as you can see...

@alexander-akait
Copy link
Member

I tried to solve it, but no luck, ideally we should move CssModules and CssDependecy on top, but we can't do it due design...

@barak007
Copy link
Contributor Author

barak007 commented Jan 17, 2022

I see. I also missed the actual private in the d.ts. checking...I didn't realized it's generated.

@alexander-akait
Copy link
Member

If you found how we can solve it or ideas, I will fix and merge it

@barak007
Copy link
Contributor Author

I have a fix but I got issues with the pre-commit hook lint-staged

[STARTED] Preparing...
[SUCCESS] Preparing...
[STARTED] Running tasks...
[STARTED] Running tasks for *
[STARTED] Running tasks for *.{js,ts}
[STARTED] prettier --write --ignore-unknown
[STARTED] eslint --cache --fix
[FAILED] �[91meslint --cache --fix�[39m �[2m[FAILED]�[22m
[SUCCESS] prettier --write --ignore-unknown
[SUCCESS] Running tasks for *
[SUCCESS] Running tasks...
[STARTED] Applying modifications...
[SKIPPED] Skipped because of errors from tasks.
[STARTED] Reverting to original state because of errors...
[SUCCESS] Reverting to original state because of errors...
[STARTED] Cleaning up...
[SUCCESS] Cleaning up...

�[91m�[91m✖�[91m eslint --cache --fix:�[39m

C:\projects\mini-css-extract-plugin\public.d.ts
  1:4  error  Parsing error: Missing semicolon. (1:4)

C:\projects\mini-css-extract-plugin\types\index.d.ts
  1:7  error  Parsing error: Unexpected token, expected "{" (1:7)

Not sure what's the problem there I can't identify any problem with the code.

I can force push if you want to check it.

@alexander-akait
Copy link
Member

skip hooks, I will fix it

@alexander-akait
Copy link
Member

Just send code

@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #900 (7c6ffbc) into master (e4f4c1e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #900   +/-   ##
=======================================
  Coverage   91.23%   91.23%           
=======================================
  Files           5        5           
  Lines         821      821           
  Branches      219      219           
=======================================
  Hits          749      749           
  Misses         66       66           
  Partials        6        6           
Impacted Files Coverage Δ
src/index.js 96.25% <ø> (ø)
src/loader.js 88.95% <ø> (ø)

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 e4f4c1e...7c6ffbc. Read the comment docs.

@barak007
Copy link
Contributor Author

I'm sorry I did only the basic type I think the static methods still needs to typed in the public.d.ts.

@alexander-akait alexander-akait mentioned this pull request Jan 17, 2022
6 tasks
@alexander-akait
Copy link
Member

@barak007 Found better way #901, anyway we have some problems on webpack side (using your implementation also have these problems):
1 Source is private and should be exported by webpack (now use any), feel free to send a PR
2. Problems with null/undefined (we should enable strict mode), it is not big problems, just can be misleading in some situations (recently we already fix these problems for callback)

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