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

Stop adding line number comments to linked JavaScript files. #9323

Merged
merged 2 commits into from Nov 8, 2017

Conversation

@benjamn
Copy link
Member

@benjamn benjamn commented Nov 8, 2017

Several years ago, before all major browsers supported source maps, we felt it was important to provide line number information in generated files using end-of-line comments like // 123\n.

Adding all these comments was always slower than leaving the code unmodified, and recently they have begun interacting badly with certain newer ECMAScript syntax, such as multi-line template strings (#9160).

Since source maps are well supported in most browsers that developers are likely to be using for development, and the line number comments are now causing substantive problems beyond the performance cost, I think it's time we stopped using them once and for all.

Fixes #9160.

Several years ago, before all major browsers supported source maps, we
felt it was important to provide line number information in generated
files using end-of-line comments like "// 123\n".

Adding all these comments was always slower than leaving the code
unmodified, and recently they have begun interacting badly with certain
newer ECMAScript syntax, such as multi-line template strings (#9160).

Since source maps are well supported in most browsers that developers are
likely to be using for development, and the line number comments are now
causing substantive problems beyond the performance cost, I think it's
time we stopped using them once and for all.

Fixes #9160.
@benjamn benjamn added this to the Release 1.6.1 milestone Nov 8, 2017
@benjamn benjamn self-assigned this Nov 8, 2017
Copy link
Member

@hwillson hwillson left a comment

This is awesome @benjamn! I've poured a 🍺 out for line number comments.

@abernix
abernix approved these changes Nov 8, 2017
Copy link
Member

@abernix abernix left a comment

/*********/ 
/* LGTM! */ 1
/*********/ 
@benjamn benjamn merged commit 35e70fd into devel Nov 8, 2017
1 of 5 checks passed
1 of 5 checks passed
ci/circleci: Get Ready Your tests are queued behind your running builds
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@apollo-cla
CLA Author has signed the Meteor CLA.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants