Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

feat: add fixWebpackSourcePath option (options.fixWebpackSourcePath) #70

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jvanbruegge
Copy link

@jvanbruegge jvanbruegge commented Aug 3, 2017

This allows to use the fixWebpackSourcePath option without karma.

Issues

edited by @michael-ciniawsky (Formatting)

@jsf-clabot
Copy link

jsf-clabot commented Aug 3, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #70 into master will decrease coverage by 15.83%.
The diff coverage is 25%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #70       +/-   ##
===========================================
- Coverage    92.3%   76.47%   -15.84%     
===========================================
  Files           2        2               
  Lines          13       17        +4     
  Branches        2        3        +1     
===========================================
+ Hits           12       13        +1     
- Misses          1        4        +3
Impacted Files Coverage Δ
src/index.js 75% <25%> (-16.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df7a77d...25e8619. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #70 into master will increase coverage by 2.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage    92.3%   94.44%   +2.13%     
==========================================
  Files           2        2              
  Lines          13       18       +5     
  Branches        2        3       +1     
==========================================
+ Hits           12       17       +5     
  Misses          1        1
Impacted Files Coverage Δ
src/index.js 94.11% <100%> (+2.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df7a77d...7cd2ec7. Read the comment docs.

@jvanbruegge
Copy link
Author

I've added a test, but I was not yet able to find a minimal webpack config that causes the paths to be wrong

@michael-ciniawsky michael-ciniawsky changed the title Add fixWebpackSourcePath option feat: add fixWebpackSourcePath option (options.fixWebpackSourcePath) Aug 31, 2017
@michael-ciniawsky michael-ciniawsky added this to the 3.1.0 milestone Aug 31, 2017
@michael-ciniawsky
Copy link
Member

Can you please give a small example?

@mattlewis92
Copy link
Member

I think a cleaner solution to this may be to change the path that is passed to istanbul, currently this.resourcePath is used which includes all the loaders in the path etc, I'm just wondering if there is an API to just get the normal file path?

@jvanbruegge
Copy link
Author

@michael-ciniawsky I was not able to find a minimal example, I just know that the problem surfaces in my webpack boilerplate

@mattlewis92 I am not familiar with the webpack API, I just know that this PR fixes the issue (https://github.com/cyclejs-community/one-fits-all/blob/a1bd1cb67759628e6d29f0c89221c1f44b5f69cd/configs/webpack.config.test.js#L21)

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 31, 2017

this.resourcePath should be the absolute path without anything webpack specific (request)

this.request = 'loader-1!loader-2!loader-n!./path/to/file?query'
this.resource = 'path/to/file?query'
this.resourcePath = 'path//to/file'
this.resourceQuery = 'query'

Please post a before after example of the sources to clearify, I take a look at the example meanwhile :)

@jvanbruegge
Copy link
Author

jvanbruegge commented Aug 31, 2017

Weird, the bug is not reproducable any more. Neither Webpack 2 nor Webpack 3 produce this error any more, I guess I can close this then.

On the other hand, the bug was there, I got the error message:

Unable to lookup source: /home/jan/test/test1/node_modules/ifdef-loader/ifdef-loader.js?json={"PRODUCTION":true,"DEVELOPMENT":false}!/home/jan/test/test1/node_modules/tslint-loader/index.js!/home/jan/test/test1/src/components/counter.tsx(ENOENT: no such file or directory, open '/home/jan/test/test1/node_modules/ifdef-loader/ifdef-loader.js?json={"PRODUCTION":true,"DEVELOPMENT":false}!/home/jan/test/test1/node_modules/tslint-loader/index.js!/home/jan/test/test1/src/components/counter.tsx')
Error: Unable to lookup source: /home/jan/test/test1/node_modules/ifdef-loader/ifdef-loader.js?json={"PRODUCTION":true,"DEVELOPMENT":false}!/home/jan/test/test1/node_modules/tslint-loader/index.js!/home/jan/test/test1/src/components/counter.tsx(ENOENT: no such file or directory, open '/home/jan/test/test1/node_modules/ifdef-loader/ifdef-loader.js?json={"PRODUCTION":true,"DEVELOPMENT":false}!/home/jan/test/test1/node_modules/tslint-loader/index.js!/home/jan/test/test1/src/components/counter.tsx')
     at Context.defaultSourceLookup [as sourceFinder] (/home/jan/test/test1/node_modules/nyc/node_modules/istanbul-lib-report/lib/context.js:15:15)
     at Context.getSource (/home/jan/test/test1/node_modules/nyc/node_modules/istanbul-lib-report/lib/context.js:74:17)
     at Object.annotateSourceCode (/home/jan/test/test1/node_modules/nyc/node_modules/istanbul-reports/lib/html/annotator.js:175:38)
     at HtmlReport.onDetail (/home/jan/test/test1/node_modules/nyc/node_modules/istanbul-reports/lib/html/index.js:217:39)
     at Visitor.(anonymous function) [as onDetail] (/home/jan/test/test1/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:34:30)     at ReportNode.Node.visit (/home/jan/test/test1/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:123:17) 
     at /home/jan/test/test1/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:116:23
     at Array.forEach (native)     at visitChildren (/home/jan/test/test1/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:115:32)
     at ReportNode.Node.visit (/home/jan/test/test1/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:126:5)
--

@michael-ciniawsky
Copy link
Member

Did you initially use webpack =< v1.0.0 ? If I remember right there were changes to the Loader API between v1.0.0 => v2.0.0

@jvanbruegge
Copy link
Author

jvanbruegge commented Aug 31, 2017

No, 100% either 2.X or 3.X, most likely 3.X

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 31, 2017

kk, please provide e.g a Gist or the like with before/after comparison in case it should still happen in one of your projects, so we can triage and finalize the PR here. I'm blocking it meanwhile. In case your issue is resolved and this isn't needed anymore (in your opinion) please close the PR.

@jvanbruegge
Copy link
Author

jvanbruegge commented Aug 31, 2017

I just tried all beta releases of my boilerplate and was not able reproduce the bug. I will close for now, if the problem surfaces again I can repopen

@jvanbruegge
Copy link
Author

This issue just resurfaced in my boilerplate. You can see it yourself by running these commands:

npx create-cycle-app@4.1.0-rc.1 testProject --flavor cycle-scripts-one-fits-all@6.0.0-beta.2
cd testProject
npm test

after that, open coverage/index.html in the browser and click on any file

@jvanbruegge
Copy link
Author

After a bit of digging, I found this in the instrumenter output:

inputSourceMap: {
    version: 3,
    file: "/home/jan/temp/name/src/components/speaker.tsx",
    sourceRoot: "/home/jan/temp/name/",
    sources: [
        "node_modules/.registry.npmjs.org/tslint-loader/3.6.0/node_modules/tslint-loader/index.js!/home/jan/temp/name/src/components/speaker.tsx"
    ],
    names: [],
    mappings: <redacted>,
    sourcesContent: [<redacted>]
}

@codecov
Copy link

codecov bot commented Oct 7, 2018

Codecov Report

Merging #70 into master will increase coverage by 2.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage    92.3%   94.44%   +2.13%     
==========================================
  Files           2        2              
  Lines          13       18       +5     
  Branches        2        3       +1     
==========================================
+ Hits           12       17       +5     
  Misses          1        1
Impacted Files Coverage Δ
src/index.js 94.11% <100%> (+2.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe5fedb...851531b. Read the comment docs.

@jvanbruegge
Copy link
Author

No idea why the tests fail, locally they pass just fine

@jvanbruegge
Copy link
Author

I also tested my boilerplate with a temporary release of my fork, and it does indeed fix the issue as expected

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fixWebpackSourcePaths without karma
4 participants