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

Export format options, accept different import formats, fixes #63 #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eekboom
Copy link

@eekboom eekboom commented Jul 4, 2017

Options "exportAsEs6Default" and "exportAsDefault" work the same way as in html-loader.

Copy link
Owner

@WearyMonkey WearyMonkey left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -45,19 +47,26 @@ module.exports = function (content) {
.replace(new RegExp(escapeRegExp(pathSep) + '+', 'g'), pathSep);
var html;

if (content.match(/^module\.exports/)) {
if (content.match(/(?:^module\.exports)|(?:^export\s+default)|(?:^exports.default)/)) {
Copy link
Owner

Choose a reason for hiding this comment

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

/^(module\.exports|export\s+default|exports.default)/ is a bit easier to read.

var firstQuote = findQuote(content, false);
var secondQuote = findQuote(content, true);
html = content.substr(firstQuote, secondQuote - firstQuote + 1);
} else {
html = content;
}

var exportsString = "module.exports = ";
Copy link
Owner

Choose a reason for hiding this comment

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

Would a simpler solution be to just keep the same export expression of the content? That way only html-loader would need to be configured.

I'm thinking something like:

var exportExprMatch = content.match(/^(module\.exports\s+=|export\s+default|exports.default\s+=)\s+/);
var exportExpr;
if (exportExprMatch) {
  var firstQuote = findQuote(content, false); 
  var secondQuote = findQuote(content, true);
  html = content.substr(firstQuote, secondQuote - firstQuote + 1);
  exportExpr = exportExprMatch[0];
} else {
  html = content;
  exportExpr = 'module.exports = ';
}

return "var path = '"+jsesc(filePath)+"';\n" +
        "var html = " + html + ";\n" +
        (requireAngular ? "var angular = require('angular');\n" : "window.") +
        "angular.module('" + ngModule + "').run(['$templateCache', function(c) { c.put(path, html) }]);\n" +
        exportExpr + "path;";

The README change would also then not be required.

Copy link
Author

Choose a reason for hiding this comment

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

Works for me!
OTOH it feels more natural to me to specify the export format at that loader whose output is actually consumed by the app.
Also that would not work with raw-loader, because that loader is hard coded to commonjs exports.
I don't know how important that scenario is, because I am new at webpack. Are there valid reasons for chossing the raw-loader over the html-loader?

@WearyMonkey
Copy link
Owner

Also could you add a test please.

@eekboom
Copy link
Author

eekboom commented Jul 5, 2017

About the test: How do they work?
Just "npm run test" and then open test/index.html in the browser and look at the console?
Currently for me two of the console.log() statements in test/src/entry.js throw an exception anyway.

How do I test ES6 imports at all?

@mikila85
Copy link

Is this still not working?
no support for type script?

@stevenvachon
Copy link

stevenvachon commented Oct 16, 2018

Anything happening here? I'm having to use @eekboom's fork, which is not as stable as an official release.

@stevenvachon
Copy link

@WearyMonkey hello?

@eekboom why did you remove your repository?

stevenvachon added a commit to vallejos/ngtemplate-loader that referenced this pull request Oct 24, 2018
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.

4 participants