Skip to content

Conversation

@tromey
Copy link
Contributor

@tromey tromey commented Sep 22, 2017

No description provided.

@tromey
Copy link
Contributor Author

tromey commented Sep 22, 2017

Dang it, those patches shouldn't be there.

@tromey tromey requested a review from gregtatum September 22, 2017 18:59
@gregtatum gregtatum changed the title Issue 227 Issue #227 Sep 22, 2017
Copy link
Member

@gregtatum gregtatum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your fix for this, but I would also think carefully about how this change affects our end users on npm, as this technically requires a major version bump since the output here is subtly different.

assert.equal(i, 1);
};

exports['test from issue 227'] = function (assert) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would appreciate a little more verbosity on the test name. test non-normalized sourceRoot values (from issue #227).

sourceRoot: './src/',
sourcesContent: [ 'var name = "Mark"\n' ]
});
consumer.sourceContentFor(consumer.sources[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like to see an assertion for the new behavior you've introduced here, since the output will not be exactly the same.

assert.equal(consumer.sourceRoot, 'src/');

This calls normalize on the provided sourceRoot in
BasicSourceMapConsumer.  This allows a funny sourceRoot
like "./src".
Fixes mozilla#227
@tromey tromey merged commit c898668 into mozilla:master Sep 25, 2017
@tromey tromey deleted the issue-227 branch September 25, 2017 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants