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

#1 only import the cssParse function we need #8

Merged

Conversation

IanGrainger
Copy link
Contributor

@IanGrainger IanGrainger commented Dec 9, 2020

What:

We can't import the CSS library in the browser (which is how Karma runs the tests) - because of an error Module not found: Error: Can't resolve 'fs'. But as suggested by a developer on that project, we can just import the function we need from the library.

Why:

Because attempting to run tests with Karma results in an error: Module not found: Error: Can't resolve 'fs'.

How:

Import the cssParse function we need rather than the whole css library.

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

@IanGrainger
Copy link
Contributor Author

Oh dear. Looks like a caching problem in your build script? :(

Copy link

@ogonkov ogonkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to see it merged

import { checkHtmlElement, getTag, InvalidCSSError } from './utils';
import { printSecSuccess, printSuccess, printSecError, printError } from './printers';
import cssParse from 'css/lib/parse';
Copy link

@ogonkov ogonkov Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that should be on the first line in this file, since it is module import, then own dependencies goes

@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #8 (5eb10da) into master (2c2ff96) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #8   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files          21       21           
  Lines         596      596           
=======================================
  Hits          593      593           
  Misses          3        3           
Impacted Files Coverage Δ
src/toHaveStyle.js 98.07% <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 2c2ff96...5eb10da. Read the comment docs.

@brrianalexis
Copy link
Member

Oh dear. Looks like a caching problem in your build script? :(

The Office - MIchael Scott -I Don't Know What The Fuck That Was
Maybe running actions on windows wasn't such a great idea 😅

I'll try this out on the weekend @IanGrainger! Let's hope for the best.

Thank you for your support and your efforts.

@IanGrainger
Copy link
Contributor Author

I've pushed changing the import order. Sorry about that! 😅

Hey, how'd the new job go? Did you get it?? 😄

@brrianalexis brrianalexis self-requested a review December 13, 2020 19:33
@brrianalexis
Copy link
Member

I did get a new job, thanks for asking! 😄

Thank you for putting so much work into this.

@brrianalexis brrianalexis merged commit 5db8663 into testing-library:master Dec 13, 2020
@brrianalexis
Copy link
Member

@all-contributors add @IanGrainger for code

@github-actions
Copy link

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@allcontributors
Copy link
Contributor

@brrianalexis

I've put up a pull request to add @IanGrainger! 🎉

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

Successfully merging this pull request may close these issues.

3 participants