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

feat: Replace mapFileDir argument with a function for reading the source map #76

Merged
merged 7 commits into from
Oct 17, 2022

Conversation

prantlf
Copy link
Contributor

@prantlf prantlf commented Jan 15, 2022

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.
  • Concatenate 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.

Fixes #64 with a breaking change on the API.

…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.
@phated
Copy link
Collaborator

phated commented Oct 15, 2022

@thlorenz I rebased this PR on all the changes in master and cleaned up this implementation so it no longer needs mapFileDir. Please take a couple minutes to review and let me know what you think. If this looks good, I think we can merge it and ship a 2.0.0

@phated phated requested a review from thlorenz October 15, 2022 23:40
@phated phated changed the title feat: Let a function for reading the source map be specified instead of depending on fs feat: Replace mapFileDir argument with a function for reading the source map Oct 15, 2022
@phated
Copy link
Collaborator

phated commented Oct 15, 2022

This also closes #52 with my changes.

Copy link
Owner

@thlorenz thlorenz left a 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.

phated and others added 3 commits October 17, 2022 13:10
@phated phated requested a review from thlorenz October 17, 2022 20:46
Copy link
Owner

@thlorenz thlorenz left a 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!

@phated phated requested a review from thlorenz October 17, 2022 21:33
@phated phated merged commit 2320633 into thlorenz:master Oct 17, 2022
@phated
Copy link
Collaborator

phated commented Oct 17, 2022

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 🙇

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