-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: add a sources option allowing to bypass fs operations #36
Conversation
Pull Request Test Coverage Report for Build 136
💛 - Coveralls |
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.
This is great, I'm ecstatic to have another party contributing to this codebase.
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.
wouldn't complain if we add a test eventually 😄 but this looks reasonable to me.
Definitely needs a test or two! I'll come back to it once we've settled on an API. This one feels a bit brute force. I think maybe some of this sourcemap logic could move to |
// both the transpiled and original source, both of which are used during | ||
// the backflips we perform to remap absolute to relative positions. | ||
const rawSourceMap = convertSourceMap.fromSource(rawSource) || convertSourceMap.fromMapFileSource(rawSource, dirname(this.path)) | ||
const rawSource = this.sources.source || readFileSync(this.path, 'utf8') |
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.
When this.sources.source
is a zero-length string, it still tries to read an actual file. I guess you meant
this.sources.source === undefined ? readFileSync(this.path, 'utf8') : this.sources.source
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.
good call!
FWIW, I'm still not happy with the branching logic I introduced in this PR, I'd prefer an explicit .loadFromFs
and .loadFromMemory
or something API. Haven't had time to dig further into this yet though
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.
👍 Separate APIs really sound good to me.
This is a first, quick, pass for #33. It does not have docs or tests as I'd like to get some feedback on it if it's the correct approach, or if we should have different
load
methods depending on if we want to read from fs or notFixes #33
(includes #35)