-
-
Notifications
You must be signed in to change notification settings - Fork 57
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: Replace mapFileDir argument with a function for reading the source map #76
Conversation
…of depending on fs This is a part of changes to get this module usable in the web browser by not depending on any Node.js modules. * Remove the fs and path dependencies. * Expect a function for reading the source map content to be passed as a parameter. * Concatenare directory and file names without path.resolve. * Behave synchronously or asynchronously depending if the source map reading method returned a string or a promise resolving to string. BREAKING CHANGE: Methods fromMapFileComment and fromMapFileSource require an additional parameter - readMap - to read the source map content. They behave synchronously or asynchronously depending on the behaviour of the readMap function.
41a07be
to
596e2b4
Compare
6c195a2
to
374a5fb
Compare
@thlorenz I rebased this PR on all the changes in master and cleaned up this implementation so it no longer needs |
This also closes #52 with my changes. |
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 very much like the end goal to make this work better in the browser.
However the way it is implemented currently will break a bunch of tools depending on this when running build scripts and such via Node.js.
The upgrade path is not trivial/obvious and thus I'm afraid most will just end up staying on the previous major if we publish as is.
I'd prefer a solution that keeps the old behavior working and warns with a deprecate message instead. We should also provide a utility function that basically is a read
function that just takes a dir
and thus makes getting rid of that deprecate message easier. The message obviously would point to the README where we explain how to use it.
Then in the next major version we can fully deprecate the old behavior.
Co-authored-by: Thorsten Lorenz <thlorenz10@gmail.com>
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.
Aside from that error printing nit LGTM.
Please improve that since without a stack users will be at a loss where this comes from especially when this module is a nested dep.
After that we can merge. Thanks for your work!
Massive shoutout to @prantlf who did all the hard work on these PRs! Sorry it took us so long to get around to reviewing them but you've made this library a ton better 🙇 |
This is a part of changes to get this module usable in the web browser
by not depending on any Node.js modules.
returned a string or a promise resolving to string.
BREAKING CHANGE: Methods fromMapFileComment and fromMapFileSource
require an additional parameter - readMap - to
read the source map content. They behave synchronously
or asynchronously depending on the behaviour
of the readMap function.
Fixes #64 with a breaking change on the API.