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

templateUrl Regex fails to capture when comment follows #58

Open
bahaabeih opened this issue Feb 13, 2017 · 11 comments
Open

templateUrl Regex fails to capture when comment follows #58

bahaabeih opened this issue Feb 13, 2017 · 11 comments

Comments

@bahaabeih
Copy link

bahaabeih commented Feb 13, 2017

In the following legitimate code snippet:

@Component({
    selector: "my-app",
    templateUrl: './app.component.html'
    //styleUrls: ["app.component.css"]
})

or this:

@Component({
    selector: "my-app",
    templateUrl: './app.component.html' //Very important comment
})

the regular expression templateUrlRegex in index.js fails to capture, leaving the URL in the file to be treated as a separate html.

@jimbarrett33
Copy link

I can also confirm that comments are OK in 0.6.0 but not in 0.6.2

@defyjoy
Copy link

defyjoy commented Feb 25, 2017

Version - "angular2-template-loader": "^0.6.2",

Probably this also has got related to the fact the templateUrl doesnt work without a comma . I found out this peculiar issue today.
This does work ---

@Component({
    selector: 'home',
    templateUrl: "./home.template.html",
})

But This does not

@Component({
    selector: 'home',
    templateUrl: "./home.template.html"
})

The only difference is the comma after templateUrl: "./home.template.html"

@GiuseppePiscopo
Copy link

Is also this issue related to #50 ?

@DOMZE
Copy link

DOMZE commented Mar 29, 2017

I can confirm what @defyjoy posted. Just tried myself. Could the regex be updated?

@Riobe
Copy link

Riobe commented Apr 27, 2017

I think my pull request #67 will fix this. If anyone else wants to try using that regex in their node_modules/angular2-template-loader/index.js then just replace line 5 with:

var templateUrlRegex = /templateUrl\s*:(\s*['"`](.*?)([^\\]['"`])\s*)/gm;

If there's any cases it doesn't work in, I'd be happy to spend some time trying to make it work for those too.

@charlie-elverson
Copy link

charlie-elverson commented Apr 27, 2017

@Riobe I swapped in your regex and it seems to have solved the issue. I only had the problem when templateUrl was the final member and was followed by a comment such as:
`templateUrl: './ES-Building-EAL.component.html'

//styleurls: ...`

@Riobe
Copy link

Riobe commented Apr 27, 2017

@charlie-elverson Glad it fixed your case. :)

I think I added a test case that matches that, because it seems like a common case (I hit it too), here

@charlie-elverson
Copy link

@Riobe Would it be worthwhile to also add a test case where the comment is on a new line? I'm terrible at regex, so I'm not sure if that requires a separate test case.

@Riobe
Copy link

Riobe commented Apr 27, 2017

@charlie-elverson Looks like the common case is someone with a commented styleUrls so they take the comma out and it explodes. So I went ahead and added a test for that here. I think that would be functionally covered by the template last test, but it's better to be sure. :)

Of interest to me is that it actually changes the styleUrls property in the comment. I can't figure out how changing the output bundle's comment for that would cause an issue though, so I figure it's ok.

@winnemucca
Copy link

Is there any new reason for this. I see that @Riobe made a recommendation that exists in the current index.js file. Those are now the same. My file's see require and error out. The link on the docs for This is caused by the way the template loader works Is broken.

@Riobe
Copy link

Riobe commented Jul 24, 2017

I don't know what you mean new reason for this. My suggestion is in pull request #67 and I showed how you could hand modify this package to include it. @TheLarkInn hasn't replied to my PR yet so it's not in master and I don't know if it ever will be at this point since I created it in April.

My fix doesn't solve the issue the way he wanted, which was a more robust technique using an AST, but it would work for the cases I've seen, his previous tests, and the new ones I've added. If you want, you can fork the project and include it, but I have no idea if this project will get updated anymore.

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

No branches or pull requests

8 participants