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

feat: #1371 Append the file extension to the mapping files in devtools #1372

Merged
merged 3 commits into from
Aug 16, 2018

Conversation

YuJianrong
Copy link

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:
image

css-module loader will generate the module class with new file path with style extension.
@yyx990803
Copy link
Member

Where does the result screenshot comes from? I'm afraid I don't understand how this affects debugging.

@YuJianrong
Copy link
Author

Consider there is a vue file App.Vue with script:

image

I want to set a breakpoint on Line 17 for debugging, but after I press ⌘-P and fill App.vue in the search input of chrome devtools, that's what I got:

image

I must open the files one by one to find the one with TypeScript for debugging:

image

That's the file search dialog after this PR:

image

Another benefit is the style file can be located easily too if sourcemap of CSS is enabled:

image

@yyx990803
Copy link
Member

yyx990803 commented Aug 13, 2018

I'm not sure if mutating this.resourcePath in a loader is considered safe practice... /cc @sokra?

@YuJianrong
Copy link
Author

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 css-module loader create the scoped class name from the path and name of the file (without extension) so the name css-module(came from css-module.vue) changed to css-module-vue(from css-module.vue.scss). I'm not sure if there will be any other side-effect from other loaders.

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.

@sokra
Copy link

sokra commented Aug 14, 2018

Mutating loaderContext.resourcePath doesn't affect webpack itself. This property is only copied to the loader context, but not read back. It only affects the loaders following in the loader chain. In this case it seem to affect the names in the generate SourceMap which is picked up by webpack. I would say it's hacky but seem "safe". It probably only affects devtools which uses the SourceMaps returned by loaders.

btw. you can give module other names with this syntax:

see also

`import block${i} from ${stringifyRequest(src + query)}\n` +

import block0 from "./src/App.vue.ts!=!vue-loader?...!./src/App.vue"

This will also pick up loaders registered for /\.ts$/. vue-loader should only extract the TS part in this case, but doesn't compile TS to JS.

The resulting module will be handled like ./src/App.vue.ts would exist.

@yyx990803
Copy link
Member

@sokra thanks for the explanation! Is the != syntax documented anywhere? Which version of webpack did it land in?

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')
Copy link
Member

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

Copy link
Author

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?

Copy link
Member

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.
@sokra
Copy link

sokra commented Aug 14, 2018

Which version of webpack did it land in?

v4.11.0

Is the != syntax documented anywhere?

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.

@yyx990803 yyx990803 merged commit 0c2da0f into vuejs:master Aug 16, 2018
@zhangyuheng
Copy link

zhangyuheng commented Aug 22, 2018

hi @YuJianrong @yyx990803 This may break the istanbul coverage html reporter, since babel-plugin-istanbul will receive absolute path path/to/file.vue.js as file path key, but the real path is path/to/file.vue.

the resourcePath is used by the test coverage reporter to locate the source file in local disk

Error detail

 { Error: ENOENT: no such file or directory, open '/builds/path/to/src/components/some-module.vue.js'

You can test the generated file path: install babel-plugin-istanbul and put this in .babelrc

      "plugins": [
        "istanbul"
      ]

then check window.__coverage__ in browser console

Maybe we can add a new option for this feature, and not enable by default

@zhangyuheng
Copy link

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
  }
}

@zhangyuheng
Copy link

zhangyuheng commented Aug 23, 2018

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;
  }
});

@EugeneJao
Copy link

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.
the exceptions:
23 08 2018 19:45:00.996:ERROR [coverage]: { Error: ENOENT: no such file or directory, open '/Users/Project/platform/frontend/src/App.vue.js'
at Object.fs.openSync (fs.js:667:18)
at Object.fs.readFileSync (fs.js:572:33)
at LookupStore.get (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/store/fslookup.js:40:19)
at HtmlReport.writeDetailPage (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/report/html.js:412:67)
at /Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/report/html.js:491:26
at SyncFileWriter.writeFile (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/util/file-writer.js:57:9)
at FileWriter.writeFile (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/util/file-writer.js:147:23)
at /Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/report/html.js:489:24
at Array.forEach ()
at HtmlReport.writeFiles (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/report/html.js:483:23)
at /Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/report/html.js:485:22
at Array.forEach ()
at HtmlReport.writeFiles (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/report/html.js:483:23)
at HtmlReport.writeReport (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/report/html.js:568:14)
at LcovReport.writeReport (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/istanbul/0.4.5/node_modules/istanbul/lib/report/lcov.js:55:19)
at writeReport (/Users/Project/platform/frontend/node_modules/.registry.npmjs.org/karma-coverage/1.1.2/node_modules/karma-coverage/lib/reporter.js:68:16)
at /Users/Project/platform/frontend/node_modules/.registry.npmjs.org/karma-coverage/1.1.2/node_modules/karma-coverage/lib/reporter.js:297:11
at /Users/Project/platform/frontend/node_modules/.registry.npmjs.org/karma/1.7.1/node_modules/karma/lib/helper.js:145:7
at /Users/Project/platform/frontend/node_modules/.registry.npmjs.org/graceful-fs/4.1.11/node_modules/graceful-fs/polyfills.js:287:18
at FSReqWrap.oncomplete (fs.js:171:5)
errno: -2,
code: 'ENOENT',
syscall: 'open',
path: '/Users/Project/platform/frontend/src/App.vue.js' }

@YuJianrong
Copy link
Author

@zhangyuheng Can you create a github issue so we can track this issue better? Thanks.
@yyx990803 Can you tell me if there is any flag/environment variable I can use to test if the loaded in running in dev mode or unit test mode to turn off this feature? Or it's better to revert it?

@zhangyuheng
Copy link

@YuJianrong

have created #1388

@Justineo
Copy link
Member

@zhangyuheng Please do read the issue template and create issues with issue helper. Otherwise your issue will be automatically closed by the bot.

@yyx990803
Copy link
Member

@YuJianrong the user may run the unit tests with NODE_ENV=development, so I'm afraid there isn't an absolute safe way to differentiate the two.

What I can think of is make this behavior an opt-in, e.g. only enabled with VUE_LOADER_APPEND_EXT=true.

@bobbylight
Copy link

I think this may have broken the ability to run tslint over *.vue files loaded with vue-loader. See my last comment here:

#1304

@yyx990803
Copy link
Member

This feature is now only enabled when explicitly passing appendExtension: true via loader options in 15.4.1 and above.

@lsycxyj
Copy link

lsycxyj commented Feb 19, 2019

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?

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.

None yet

8 participants