← 输入框|github.com|刷新
Skip to content

fix(compiler): Don't strip /*# sourceURL ... */ - #16088

Closed
asnowwolf wants to merge 1 commit into
angular:masterfrom
asnowwolf:dont-strip-source-url-comment
Closed

fix(compiler): Don't strip /*# sourceURL ... */#16088
asnowwolf wants to merge 1 commit into
angular:masterfrom
asnowwolf:dont-strip-source-url-comment

Conversation

@asnowwolf

@asnowwolf asnowwolf commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

Currently, shimCssText only keep /*# sourceMappingUrl ... */ comments and strip /*# sourceURL ... */ comments. So, Chrome can't find the source maps for component style(that's created in new style tags)

What is the new behavior?

Don't strip /*# sourceURL ...*/ comments. Chrome will find source maps for component styles properly.

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@asnowwolf asnowwolf closed this Apr 18, 2017
@asnowwolf asnowwolf reopened this Apr 18, 2017
@asnowwolf
asnowwolf force-pushed the dont-strip-source-url-comment branch from 8b4180d to 6fe610a Compare April 18, 2017 14:50
@asnowwolf asnowwolf mentioned this pull request Apr 18, 2017
Closed
2 of 3 tasks
@asnowwolf

asnowwolf commented Apr 18, 2017

Copy link
Copy Markdown
Contributor Author

Sorry, the network of my home is too slow, I'll resolve the CI failure tomorrow.

@vicb vicb mentioned this pull request Apr 18, 2017
Closed
4 of 12 tasks
@asnowwolf
asnowwolf force-pushed the dont-strip-source-url-comment branch 3 times, most recently from 971b97f to 8ad81c3 Compare April 19, 2017 06:02
@vicb vicb added the area: core Issues related to the framework runtime label Apr 26, 2017
@asnowwolf asnowwolf mentioned this pull request Jun 29, 2017
Closed
@OzzieOrca OzzieOrca mentioned this pull request Jul 8, 2017
Closed
@daddyschmack

daddyschmack commented Jul 8, 2017

Copy link
Copy Markdown

Hi Guys, how long until this get merged in? The lack of source maps is making my job MUCH more difficult.

@daddyschmack

daddyschmack commented Aug 10, 2017

Copy link
Copy Markdown

Guys, did the fix make it in to 4.3.3? If not, when can you implement?

@asnowwolf

asnowwolf commented Aug 10, 2017

Copy link
Copy Markdown
Contributor Author

@tbosch @vicb @chuckjaz What should I do more to make this PR get merged?

@asnowwolf asnowwolf mentioned this pull request Aug 15, 2017
Merged
4 of 12 tasks
@filipesilva filipesilva mentioned this pull request Oct 27, 2017
Closed
1 of 2 tasks
vicb
vicb reviewed Jan 16, 2018
View reviewed changes
Comment thread packages/compiler/src/shadow_css.ts Outdated

@vicb vicb Jan 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should only match what is intended ie
/\/\*\s*#\s*source(Mapping)?URL=[\s\S]+?\*\//

vicb
vicb reviewed Jan 16, 2018
View reviewed changes
Comment thread packages/compiler/src/shadow_css.ts Outdated

@vicb vicb Jan 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

return input.match(...) || [] and remove the null from the return type and simplify the usage

@vicb vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 16, 2018
@vicb vicb self-assigned this Jan 16, 2018
@vicb

vicb commented Jan 16, 2018

Copy link
Copy Markdown
Contributor

@asnowwolf Sorry that it took so long. Could you please rebase and fix the comments so that we can merge it. Thanks.

@ngbot

ngbot Bot commented Jan 17, 2018

Copy link
Copy Markdown

Hello? Don't want to hassle you. Sure you're busy. But this PR has some merge conflicts that you probably ought to resolve.
That is... if you want it to be merged someday...

@asnowwolf @vicb
fix(compiler): Don't strip /*# sourceURL ... */
783cfcd 
Currently, `shimCssText` only keep `/*# sourceMappingUrl ... */` comments and strip `/*# sourceURL ... */` comments. So, Chrome can't find the source maps for component style(that's created in new `style` tags)
@vicb
vicb force-pushed the dont-strip-source-url-comment branch from 8ad81c3 to 783cfcd Compare January 24, 2018 18:30
@vicb vicb added target: patch This PR is targeted for the next patch release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 24, 2018
@vicb

vicb commented Jan 24, 2018

Copy link
Copy Markdown
Contributor

@asnowwolf I have updated your branch to fix my comments. Thanks for your initial work on this and sorry again for the delay in merging this.

@vicb vicb added the action: merge The PR is ready for merge by the caretaker label Jan 24, 2018
mhevery pushed a commit that referenced this pull request Jan 24, 2018
@asnowwolf @mhevery
fix(compiler): Don't strip /*# sourceURL ... */ (#16088)
de6c644 
Currently, `shimCssText` only keep `/*# sourceMappingUrl ... */` comments and strip `/*# sourceURL ... */` comments. So, Chrome can't find the source maps for component style(that's created in new `style` tags)

PR Close #16088
@mhevery mhevery closed this in 5f681f9 Jan 24, 2018
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 13, 2019

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@vicb vicb vicb approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@vicb vicb

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@asnowwolf @daddyschmack @vicb @chuckjaz @googlebot