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

Sourcemap corrections #1750

Closed
wants to merge 2 commits into from
Closed

Conversation

johnjbarton
Copy link
Contributor

clarify --sourceroot option meaning.

Correct relativePath to work for dist/js/filename case.
Add test.

Don't mark the placeholder parser source as input source

Fixes #1685

@@ -58,6 +58,12 @@ names.forEach(function(name) {
});
print(' */');
print(' constructor(%s) {', paramNames.join(', '));
if (paramNames[0] === 'location') {
print('if (location) {');
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the perf implication?

Maybe move this into a function so it is not repeated all over.

This should be location !== null since passing undefined is an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I should take this out and re-do this as a pass in test-utils. As the feature tests run they should create an instance of every type and those trees can be checked. Slower only for feature tests that pass. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeas. Making this a test only thing would be good.

@arv
Copy link
Collaborator

arv commented Feb 19, 2015

Just ping me when this is ready for review

@johnjbarton
Copy link
Contributor Author

PTAL

@jmm
Copy link

jmm commented Feb 19, 2015

clarify --sourceroot option meaning.
sourcemap sourceRoot value

So the --source-root value will now simply be passed through as the sourceRoot output in the source map and not treated as a local path?

@johnjbarton
Copy link
Contributor Author

So the --source-root value will now simply be passed through as the sourceRoot output in the source map and not treated as a local path?

The sourceroot code is not changed in this Pull Request. I change the description emitted by the command line help. The --source-root value can be true for basepath of the output file name, false to omit, or a string to be loaded in to the sourcemap.

@jmm
Copy link

jmm commented Feb 19, 2015

Ok, thanks. I just wanted to double check that documentation will be accurate once published or else it would cause more confusion.

@arv
Copy link
Collaborator

arv commented Feb 20, 2015

LGTM minus the location assertions.

Correct relativePath to work for dist/js/filename case.
Add test.

Don't mark the placeholder parser source as input source

Fixes google#1685
@arv arv removed the in progress label Feb 20, 2015
@johnjbarton johnjbarton deleted the sourcemappingurl branch May 4, 2015 23:33
@johnjbarton johnjbarton restored the sourcemappingurl branch May 12, 2015 23:32
@johnjbarton johnjbarton deleted the sourcemappingurl branch January 29, 2016 02:08
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.

Source Maps Url Problem
3 participants