-
Notifications
You must be signed in to change notification settings - Fork 918
Add support for custom blocks in .vue files #499
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
Conversation
lib/loader.js
Outdated
@@ -271,6 +273,20 @@ module.exports = function (content) { | |||
'__vue_options__.staticRenderFns = __vue_template__.staticRenderFns\n' | |||
} | |||
|
|||
// add requires for customBlocks | |||
if (parts.customBlocks && parts.customBlocks.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid adding any code to the file unless there is a matching loader for the custom block.
lib/loader.js
Outdated
@@ -155,6 +155,8 @@ module.exports = function (content) { | |||
return loader + '!' + rewriter + ensureBang(ensureLoader(lang)) | |||
case 'script': | |||
return injectString + ensureBang(ensureLoader(lang)) | |||
default: | |||
return ensureBang(loaders[type]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would probably throw if loaders[type]
doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you code updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should have replied to this. This won't throw because there's a guard on line 289 that emits a warning in this case. This is covered by the warning should be raised when a loader is not specified for a custom block
unit test.
Thanks for the PR! A few issues that need to be fixed, and ideally we need a test case for this too. |
Thanks for the feedback! I fixed the 2 issues and added them to the PR. I'll work on adding some tests and updating the documentation one night this week. |
Hmmm. I think I messed up my rebase... I'll try fixing that so the patch is cleaner... |
ff9314a
to
972181c
Compare
FYI, I haven't forgotten about building unit tests for this PR; I've just been slammed with a number of things demanding my time right now. I still intend to finish this up, targeting the week of Dec 26-30. |
5a4035f
to
0d809ec
Compare
Unit tests added, please let me know if there are any other changes required. |
@asselin this is looking great. I have one question though, would there be a way to 'ignore' certain custom blocks in different Webpack builds? Perhaps this is just me missing something in the documentation, or maybe it's something that needs to be added but, as an example, I wouldn't necessarily want to have vue-loader parsing documentation or unit-tests when I'm in the middle of development. I guess maybe with Webpack's caching system this might not make a huge performance issue, but even when thinking about CI systems, it would be nice to be able to have some way to "ignore" certain custom blocks. |
@iamkether If you want to leave some part of the .vue file out of the generated code, you can specify to use the null-loader in the webpack config file (for example, omit unit tests from debug & release builds). vue-loader will still process that section of the .vue file, but passing it to the null loader will effectively make it cease processing there. |
@yyx990803 or @kazupon: This PR is ready for a 2nd review (and merge, hopefully). |
@asselin ah, okay, that makes sense, thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed done 👀
please check my comments 💬
docs/en/start/spec.md
Outdated
@@ -22,6 +22,10 @@ export default { | |||
color: red; | |||
} | |||
</style> | |||
|
|||
<custom1> | |||
This could be e.g. documentation for the component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need indent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was indented by a tab. Changed to 2 spaces.
docs/en/start/spec.md
Outdated
@@ -66,6 +70,66 @@ More details can be found in [Using Pre-Processors](../configurations/pre-proces | |||
|
|||
- By default, contents will be extracted and dynamically inserted into the document's `<head>` as an actual `<style>` tag using `style-loader`. It's also possible to [configure Webpack so that all styles in all components are extracted into a single CSS file](../configurations/extract-css.md). | |||
|
|||
### Custom Blocks | |||
|
|||
Additional custom blocks can be included in a `*.vue` file for any project specific needs. `vue-loader` will use the tag name to look up which webpack loaders should be applied to the contents of the section. The webpack loaders should be specified in the `loaders` hash of the `vue` section of the webpack configuration in the same way that languages are specified for the standard sections of the file. See [Advanced Loader Configuration](../configurations/advanced.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please arrange the space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not clear on what you're asking for here. Is it 1 space between sentences instead of 2? If so, I just changed that and will push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, my poor explanation. 🙇
exactly!
lib/loader.js
Outdated
@@ -155,6 +155,8 @@ module.exports = function (content) { | |||
return loader + '!' + rewriter + ensureBang(ensureLoader(lang)) | |||
case 'script': | |||
return injectString + ensureBang(ensureLoader(lang)) | |||
default: | |||
return ensureBang(loaders[type]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you code updated?
@kazupon I made updates to the documentation, and see my comment about the test not being necessary. If I didn't make the changes you were looking for on the doc, please let me know. Thanks! |
@asselin Thanks! LGTM! /ping @yyx990803 |
@yyx990803 Is this good to merge now? Any other cleanup needed? FYI, I have a follow-on PR that I'm going to submit that adds support for adding attribute values from custom blocks to the webpack command as query parms. Thanks! |
@asselin sorry for dragging on, just busy with other stuff and don't want to rush-review this. Will try to finalize this week or next. |
Rebased in on current master, and added a fix for extract-text-plugin 2.0 rc |
@yyx990803 I see that you're spending some time on vue-loader; any chance this PR could be merged soon? |
Add in additional changes that add attributes on custom blocks as query parms for the loader
f60c06d
to
002d637
Compare
Add in additional changes that add attributes on custom blocks as query parms for the loader
This builds on vuejs/vue#4157 adding support in vue-loader for custom blocks. If a custom block is found in a .vue file, the loader for it will be looked up in the webpack vue configuration block just like a language is for script/style/template.
I think this improvement could probably cover the requests in some other feature requests, such as #457, #423, #413, #223, and having the ability in a UI kit project to specify (e.g.) Sass in a .vue file for a GUI component that is meant to be extracted and concatenated, and compiled into CSS by the application that pulls in and uses the UI kit.
This is a pretty simplistic implementation-- if there are issues that you think should be addressed in order to merge this, please let me know.