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.

Sign up for GitHub

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

Jump to bottom

fix(compiler): ignore @import in comments #13368

Merged
merged 2 commits into from Dec 21, 2016
Merged

fix(compiler): ignore @import in comments #13368

merged 2 commits into from Dec 21, 2016

Conversation

DzmitryShylovich
Copy link
Contributor

Fixes #12196

vicb
vicb reviewed Dec 10, 2016
View reviewed changes
modules/@angular/compiler/test/style_url_resolver_spec.ts
it('should ignore "@import" in comments', () => {
const css = '/*@import "foo!"*/';
const styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
expect(styleWithImports.style.trim()).toEqual('');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the test should be:

expect(...).toContain(nonCommentedImport);
expect(...).not.toContain(commentedImport);

Also please you PR two commits:

  • refactor,
  • fix.
    Thanks

vicb
vicb reviewed Dec 10, 2016
View reviewed changes
modules/@angular/compiler/src/style_url_resolver.ts
// remove comments
cssText = cssText.replace(CSS_COMMENT_REGEXP, '');

const modifiedCssText = cssText.replace(CSS_IMPORT_REGEXP, (...m: string[]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also chain replace

@vicb vicb added area: core Issues related to the framework runtime action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews pr_state: LGTM labels Dec 10, 2016
@DzmitryShylovich
Copy link
Contributor Author

DzmitryShylovich commented Dec 10, 2016 •
edited

@vicb comments are addressed

@DzmitryShylovich
Copy link
Contributor Author

@vicb ping

@vicb vicb added action: merge The PR is ready for merge by the caretaker 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 Dec 20, 2016
@vicb
Copy link
Contributor

vicb commented Dec 20, 2016

Thanks !

@chuckjaz chuckjaz merged commit c0f750a into angular:master Dec 21, 2016
@DzmitryShylovich DzmitryShylovich deleted the gh/12196 branch December 21, 2016 07:28
chuckjaz pushed a commit to chuckjaz/angular that referenced this pull request Dec 21, 2016
@chuckjaz
fix(compiler): ignore @import in comments (angular#13368)
6316e5d 
* refactor(compiler): clean up style url resolver
* fix(compiler): ignore @import in css comments

Closes angular#12196
@angular-automatic-lock-bot
Copy link

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 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vicb vicb vicb left review comments

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The styles parser handles @import statements in comments and tries to load invalid resources
4 participants
@DzmitryShylovich @vicb @chuckjaz @googlebot