feat: ensure module paths in source map include full path from current working directory #26

bryanforbes · 2017-02-15T20:22:04Z

This pull request is similar to #9. If the loader is configured with includeModulePaths set to true, the path of the source file will be computed relative to the current working directory if possible. This maintains the default behavior for most users but allows users to opt-into this behavior.

joshwiens · 2017-03-07T03:10:58Z

@bryanforbes - To trigger the CLA assistant now that it's actually turned on, close & open the pull request again ( this one ) and please rebase with current master to pick up the Travis build.

jsf-clabot · 2017-03-08T03:36:19Z

All committers have signed the CLA.

…ing directory

…s(this)`

bryanforbes · 2017-03-08T15:52:59Z

The latest loader-utils has a deprecation warning when using parseQuery(), so I updated the code to use getOptions().

michael-ciniawsky

@bryanforbes Thx

michael-ciniawsky · 2017-03-08T17:05:01Z

@bryanforbes A quick note in the docs before merging would be appreciated :)

michael-ciniawsky · 2017-03-08T17:27:17Z

@bryanforbes In #9 was implemented without adding an extra option, so my question is would it be possible without an extra options (includeModulePath) or is this definitely breaking things then?

Off topic but could you bump the async dependency on the fly? 🙃 #7

bryanforbes · 2017-03-08T17:49:16Z

I was trying to maintain backwards compatibility with the flag so people could opt-into the new behavior (in case there were problems or someone liked all of their source mapped files strewn about). If that's not a concern, I can remove the flag and we won't need getOptions().

Yes, I can bump the async dependency on the fly. Is @latest OK?

michael-ciniawsky · 2017-03-08T17:56:57Z

I was trying to maintain backwards compatibility with the flag so people could opt-into the new behavior (in case there were problems or someone liked all of their source mapped files strewn about). If that's not a concern, I can remove the flag and we won't need getOptions().

cc @bebraw @d3viant0ne

Is @latest OK?

Thx 👍 . I think (believe) not :D .If there is the smallest problem coming up, don't bother with (it's not really your problem) 😛

bebraw · 2017-03-08T19:57:02Z

I guess ideally there would be no flag. The problem is that if we drop the flag, then it's a breaking change (major version bump).

bryanforbes · 2017-03-08T21:27:41Z

@bebraw Isn't this slated for 1.0.0 which would be a major version bump anyway?

joshwiens · 2017-03-08T22:42:18Z

@bryanforbes - Here's the thing with a major, we are standardizing all of the loaders & plugins which is going to require a bit of coordination so we don't have to release back to back majors.

One answer here is this lands without a flag as a non-backwards compatible feature ( both for the lack of flag & the loader-utils bump ) and is done conjunction with the update to webpack-defaults which enforces a minimum version of NodeJS 4.3 & exports an ES module.

When all of that is in master, it will be published as 1.0.0-beta.0 on the @beta dist-tag

michael-ciniawsky · 2017-03-08T22:52:05Z

I agree with @d3viant0ne here, but @bryanforbes please feel free to express your opinion here also, since it would again require changes to your PR, we would appreciate it though if we could tag this for 1.0.0 in a clean way :)

joshwiens · 2017-03-08T23:05:04Z

That said, this does have benefits to the 0.x.x version as well though that would require the removal of the loader-utils update ( requires NodeJS > 4.3 which is breaking ) and the flag to remain so this is an opt in feature.

Release it as a Minor and remove the flag soon after in the pending Majorwhich now that i think about it is the path which is most beneficial to everyone.

bebraw · 2017-03-10T06:42:29Z

index.js

+		if (params.includeModulePaths) {
+			// Ensure module paths include full path from current working directory
+			var pwd = process.cwd();
+			if (context.substring(0, pwd.length) === pwd) {


The if bit feels like a helper function. Probably cleaner after extracting it.

I'm not sure what you mean. Would the if be in the helper function? Is extracting ~10 lines of code to another function going to add clarity?

if (context.substring(0, pwd.length) === pwd) { felt like a helper based on the code below. Probably better wait on someone else's comment on this.

Oh, I see what you're saying. I didn't extract it out because I don't see it being used anywhere else. IMO, anonymous functions in that case. But I can extract it out if need be.

Yeah, anonymous at tops. 👍

Do you mean turn it into a named function and place it at the top of the loader function (after line 20)?

That would work. Just extracting is enough for me. 👍

I just looked again. Extracting it to line 20 isn't possible. It's using the map and context arguments from processMap(). It's similar to the callbacks passed to async.map() at line 63.

Ok, let's leave it as is. No need to get stuck here.

👍 Sounds good

bebraw · 2017-03-10T06:43:11Z

test/index.test.js

@@ -3,7 +3,7 @@ var fs = require("fs");
 var should = require("should");
 var loader = require("../");

-function execLoader(filename, callback) {
+function execLoader(filename, callback, includeModulePaths) {


I am not so sure about the flag. It would feel better to have another function (exec???Loader).

bryanforbes · 2017-03-16T16:15:56Z

@d3viant0ne I want to be sure I'm reading what you said right:

I back out loader-utils changes and keep flag
You guys release 0.X.X with those changes
I submit a new PR (or you guys update master after the 0.X.X release) to remove the flag and make the module path behavior default
You guys release 1.0.0

michael-ciniawsky · 2017-06-05T20:17:06Z

Status ? :)

swernerx · 2017-08-04T07:04:41Z

See also #50 which includes this + adds a 'quiet' option to disable emitting of warnings.

joshwiens · 2017-08-07T09:24:54Z

@bryanforbes - FYI i'm going to try and get this into a 1.0.0-beta build by the end of the week. Ignore the plan ^^ & leave loaderUtils as is.

unional · 2017-11-02T09:47:24Z

Can't up vote this enough. This is a major use case and major pain for any package author/consumer who uses TypeScript.
Is the 1.0.0 getting ready soon? 🌷

joshwiens · 2017-11-02T10:06:11Z

@bryanforbes - Can you give this a quick rebase when you have a moment please.

hipstersmoothie · 2018-04-27T18:03:36Z

I can resolve the package.json conflict if necassary ;) really hankering for this release!

This module is installed directly from a github URL pending resolution of webpack-contrib/source-map-loader#26. Signed-off-by: Neal Granger <neal@nealg.com>

alexander-akait · 2020-05-22T17:23:37Z

Fixed in the master

bryanforbes mentioned this pull request Feb 15, 2017

Webpack 2 dojo/cli-build-webpack#67

Merged

3 tasks

joshwiens self-assigned this Mar 7, 2017

joshwiens added the enhancement label Mar 7, 2017

michael-ciniawsky added type: Feature and removed enhancement labels Mar 7, 2017

bryanforbes closed this Mar 8, 2017

bryanforbes reopened this Mar 8, 2017

Ensure module paths in source map include full path from current work…

5f1f223

…ing directory

bryanforbes force-pushed the module-context branch from 463701b to 5f1f223 Compare March 8, 2017 03:39

Updated loader-utils and changed code to use `loaderUtils.getOption…

76fd5df

…s(this)`

michael-ciniawsky approved these changes Mar 8, 2017

View reviewed changes

michael-ciniawsky added the pr: merge label Mar 8, 2017

michael-ciniawsky added the pr: docs label Mar 8, 2017

michael-ciniawsky mentioned this pull request Mar 8, 2017

Include module directory name to sourcemap #9

Closed

joshwiens requested a review from bebraw March 10, 2017 05:43

bebraw reviewed Mar 10, 2017

View reviewed changes

Change test structure to eliminate flag

879655e

michael-ciniawsky changed the title ~~Ensure module paths in source map include full path from current working directory~~ feat: ensure module paths in source map include full path from current working directory Jun 5, 2017

michael-ciniawsky added priority: 4 (important) pr_action: Review semver: Major severity: 4 (inconvenient) pr_action: Rework semver: Minor status: Pending and removed pr_action: Review semver: Major labels Jun 5, 2017

michael-ciniawsky added this to the 0.3.0 milestone Jun 5, 2017

jsg2021 mentioned this pull request Aug 3, 2017

refactor: prevent source maps overwrites #8

Closed

swernerx added a commit to swernerx/source-map-loader that referenced this pull request Aug 4, 2017

Merged in changes from webpack-contrib#26

40a3514

swernerx mentioned this pull request Aug 4, 2017

Add support for 'quiet' option to disable warning output. #50

Closed

joshwiens added priority: 3 (required) and removed priority: 4 (important) severity: 4 (inconvenient) labels Aug 7, 2017

joshwiens modified the milestones: 1.0.0, 0.3.0 Aug 7, 2017

joshwiens removed their assignment Aug 17, 2017

arielschiavoni mentioned this pull request Apr 28, 2018

Prevent sourcemaps provided by other libs/deps being overwritten #63

Closed

10xjs added a commit to sensu/sensu-go that referenced this pull request May 23, 2018

Use include dependency source maps

5dd4f5e

This module is installed directly from a github URL pending resolution of webpack-contrib/source-map-loader#26. Signed-off-by: Neal Granger <neal@nealg.com>

alexander-akait closed this May 22, 2020

Uh oh!

feat: ensure module paths in source map include full path from current working directory #26

feat: ensure module paths in source map include full path from current working directory #26

Uh oh!

Conversation

bryanforbes commented Feb 15, 2017

Uh oh!

joshwiens commented Mar 7, 2017

Uh oh!

jsf-clabot commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanforbes commented Mar 8, 2017

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented Mar 8, 2017

Uh oh!

michael-ciniawsky commented Mar 8, 2017

Uh oh!

bryanforbes commented Mar 8, 2017

Uh oh!

michael-ciniawsky commented Mar 8, 2017

Uh oh!

bebraw commented Mar 8, 2017

Uh oh!

bryanforbes commented Mar 8, 2017

Uh oh!

joshwiens commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented Mar 8, 2017

Uh oh!

joshwiens commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanforbes commented Mar 16, 2017

Uh oh!

michael-ciniawsky commented Jun 5, 2017

Uh oh!

swernerx commented Aug 4, 2017

Uh oh!

joshwiens commented Aug 7, 2017

Uh oh!

unional commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshwiens commented Nov 2, 2017

Uh oh!

hipstersmoothie commented Apr 27, 2018

Uh oh!

alexander-akait commented May 22, 2020

Uh oh!

Uh oh!

jsf-clabot commented Mar 8, 2017 •

edited

Loading

joshwiens commented Mar 8, 2017 •

edited

Loading

joshwiens commented Mar 8, 2017 •

edited

Loading

unional commented Nov 2, 2017 •

edited

Loading