-
Notifications
You must be signed in to change notification settings - Fork 253
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
#139 error can't resolve 'fs' fix #146
Conversation
…nning in a browser
try { | ||
var fs = require("fs"); | ||
} catch (e) { | ||
var fs = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be check for fs !== null
in all methods, that depends on it. In case fs
is not loaded it should throw some error to make it clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've done that - there's only one place that needs changing for runtime I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now created a second PR with an alternative approach - I think I like it a little more - but up to you! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand but if it's a dep, why to not add it to the dependencies or peers first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I know what you mean. There is no equivalent to fs in the browser. But with this workaround I can parse CSS in the browser... Meaning I get to use the jasmine-dom library from Karma! 😍 So, what needs to change? 🙏
Is this repo active? I created PRs a week ago... 😦 |
2 weeks ago now... @ogonkov / @lr-mjaouen - any advice on how I can get this (or the other PR) merged would be greatly appreciated 🙏 |
Project dead? |
Yeah. Seems like it 😦 . Its obviously good enough at what it does - because there's no alternatives that I can see, either! |
facing the same problem.. |
@IanGrainger seems like this PR does not make sense. Please have a look index file: https://github.com/reworkcss/css/blob/master/index.js exports.parse = require('./lib/parse');
exports.stringify = require('./lib/stringify'); Just use // Can not use import cssParse from 'css/lib/parse' due @types/css does not have correct declarations
const cssParse = require('css/lib/parse');
// ....
const css = cssParse(inputCssText); instead of import cssParser from 'css';
// ...
var css = cssParser.parse(inputCssText); In this case you import only css parser without stringify. |
Um, I tried that and got
|
Not
But const cssParse = require('css/lib/parse'); |
Argh. I fail at copy+paste, sorry! You're right!! |
I've created a PR on the project I'm trying to fix. Thank you @Fi1osof !! testing-library/jasmine-dom#8 |
Not at all! :) |
i hide fs in my source now its started working but i just use string css so i think i don't want fs |
Catch error if
fs
module not found - as when running in a browser.This means that
parse
will work - which is all I need. Though it won't allow you to use sourcemaps!