-
Notifications
You must be signed in to change notification settings - Fork 916
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
feat: #1371 Append the file extension to the mapping files in devtools #1372
Conversation
css-module loader will generate the module class with new file path with style extension.
Where does the result screenshot comes from? I'm afraid I don't understand how this affects debugging. |
Consider there is a vue file I want to set a breakpoint on Line 17 for debugging, but after I press I must open the files one by one to find the one with TypeScript for debugging: That's the file search dialog after this PR: Another benefit is the style file can be located easily too if sourcemap of CSS is enabled: |
I'm not sure if mutating |
I must say there is already some side-effect. That's why the test case is modified in this PR too: @@ -179,6 +179,6 @@ test('CSS Modules', async () => {
// custom ident
await testWithIdent(
'[path][name]---[local]---[hash:base64:5]',
- /css-modules---red---\w{5}/
+ /css-modules-vue---red---\w{5}/
)
}) the I know this PR is very hacky, but I think the debugging experience is very important, it will be great if you can find another elegant and less side-effect way to improve the debugging experience. |
Mutating btw. you can give module other names with this syntax: see also vue-loader/lib/codegen/customBlocks.js Line 17 in cd17d05
import block0 from "./src/App.vue.ts!=!vue-loader?...!./src/App.vue" This will also pick up loaders registered for The resulting module will be handled like |
@sokra thanks for the explanation! Is the |
lib/select.js
Outdated
@@ -1,6 +1,7 @@ | |||
module.exports = function selectBlock (descriptor, loaderContext, query) { | |||
// template | |||
if (query.type === `template`) { | |||
loaderContext.resourcePath += '.' + (descriptor.template.type || 'template') |
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 should be descriptor.template.lang
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.
@yyx990803 Changed to descriptor.template.lang || 'html'
(I think the default type should be 'html'?) in the new commit. But should we switch to the !=
solution for it looks much better?
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.
That involves a bigger refactor and likely a new major version.
Update the template name to `template.lang` and use `html` by default.
v4.11.0
I don't think so. All it does is setting the internal resource name, so webpack see it as resource of this name. This has the side effect that rules matching this name are applied. |
hi @YuJianrong @yyx990803 This may break the istanbul coverage html reporter, since babel-plugin-istanbul will receive absolute path the resourcePath is used by the test coverage reporter to locate the source file in local disk Error detail
You can test the generated file path: install
then check Maybe we can add a new option for this feature, and not enable by default |
https://github.com/istanbuljs/babel-plugin-istanbul/blob/master/src/index.js#L8-L14 babel-plugin-istanbul will return a wrong path function getRealpath (n) {
try {
return realpathSync(n) || n // <= path/module.vue not found
} catch (e) {
return n // <= goes here return path/module.vue.js
}
} |
And this is also breaking https://github.com/SitePen/remap-istanbul 's source remap. It uses extname for filename replacement see https://github.com/SitePen/remap-istanbul/blob/master/src/CoverageTransformer.js#L144 And, for ts and remap-istanbul users, I found this solution SitePen/remap-istanbul#143 use mapFileName config, this will create clean test coverage report (without displaying querystring and transformed extname after source file name) again: remap(__coverage__, {
mapFileName: filename => {
const originName = filename.replace(/\.vue\.[jt]s(\?.+)?$/, '.vue');
return originName;
}
}); |
This break the istanbul coverage html reporter. babel-plugin-istanbul will receive /to/file.vue.js as file path, but the real path is file.vue. reporter cannot create the recover report. |
@zhangyuheng Can you create a github issue so we can track this issue better? Thanks. |
have created #1388 |
@zhangyuheng Please do read the issue template and create issues with issue helper. Otherwise your issue will be automatically closed by the bot. |
@YuJianrong the user may run the unit tests with What I can think of is make this behavior an opt-in, e.g. only enabled with |
I think this may have broken the ability to run tslint over *.vue files loaded with vue-loader. See my last comment here: |
This feature is now only enabled when explicitly passing |
I just met the similar problem #1495 of broken html coverage report with karma because of the weird query suffix of vue files. Any solutions? |
Improve the debugging experience for
<script>
in the.vue
file by append the type extensions to the end of files in devtools, to make file locate easier.Result: