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

fix: fix the filepath of the stack #7

Merged
merged 1 commit into from
Apr 24, 2017

Conversation

popomore
Copy link
Contributor

This error is introduced by #6

How to reproduce

// index.js
require('intelli-espower-loader');
require('./test/index.test.js');

// test/index.test.js
throw new Error(1);

run node index.js, then you can see the wrong path /Users/popomore/code/tmp/test/test/index.test.js

/Users/popomore/code/tmp/test/test/index.test.js:1
throw new Error(1);
      ^
Error: 1
    at Object.<anonymous> (/Users/popomore/code/tmp/test/test/index.test.js:1:7)
    at Module._compile (module.js:570:32)
    at Object.extensions..js (/Users/popomore/code/tmp/node_modules/.1.2.1@espower-loader/index.js:46:25)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/popomore/code/tmp/index.js:2:1)
    at Module._compile (module.js:570:32)

@shepherdwind
Copy link
Contributor

If remove this, the ts path will be wrong. For example

// index.js
require('intelli-espower-loader');
require('./test/foo.test.js');

// test/foo.test.js
throw new Error('123');
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiZm9vLnRlc3QuanMiLCJzb3VyY2VSb290IjoiIiwic291cmNlcyI6WyJmb28udGVzdC50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFJQSxNQUFNLElBQUksS0FBSyxDQUFDLEtBQUssQ0FBQyxDQUFDIn0=

and run node index.js

Error: 123
    at Object.<anonymous> (foo.test.ts:5:7)
    at Module._compile (module.js:571:32)
    at Object.extensions..js (/Users/eward/test/foo/node_modules/.npminstall/espower-loader/1.2.1/espower-loader/index.js:45:25)
    at Module.load (module.js:488:32)

@popomore
Copy link
Contributor Author

popomore commented Apr 20, 2017

@shepherdwind can you give us the code which is generating the sourcemap by ts

@popomore
Copy link
Contributor Author

It seems convert-source-map will use the sourcemap which is generated by typescript.

@shepherdwind
Copy link
Contributor

I know what is the problem now.

The first example, js file don't have sourcemap comment, so there is not source map and source path. Then the source path lookup by the function adjustFilepath. The source file path will be a relative path , for example test/index.test.js .

And the next example, js code have some source map comment. And ts source map file source just a file name ,not a path, for example index.test.ts.

I don't know if we can change adjustFilepath to return the file name?

@twada
Copy link
Member

twada commented Apr 21, 2017

@shepherdwind Would you create a small project or gist to reproduce your issue? I'll investigate the problem.

@shepherdwind
Copy link
Contributor

@twada
Copy link
Member

twada commented Apr 22, 2017

@shepherdwind Thanks for the repro case.

I'm going to accept @popomore's PR since correct and relative filepath is more important to power-assert for security reason and for users using power-assert on browsers. espower-source makes every path relative to project root (that's why adjustFilepath function exists).

SourceMap that espower-loader reads is generated by espower-source, that is reconnected with the original SourceMap by multi-stage-sourcemap. Original SourceMap may contain multiple sources stored in different directories. It's difficult to set sourceRoot after the sourcemap processing.

@shepherdwind, is there any way to make TypeScript storing more informative paths instead of basename?

@shepherdwind
Copy link
Contributor

shepherdwind commented Apr 23, 2017

Ok, I will try to do something to add more info for typescript source map. Accept this pr is fine.

@okoala
Copy link

okoala commented Apr 24, 2017

fix it quickly, Please 😖

@twada twada merged commit 4904d21 into power-assert-js:master Apr 24, 2017
@twada
Copy link
Member

twada commented Apr 24, 2017

@okoala @popomore Okay I'm going to release soon.

@twada
Copy link
Member

twada commented Apr 24, 2017

Just released espower-loader 1.2.2

@popomore popomore deleted the stack-url branch April 24, 2017 10:08
@popomore
Copy link
Contributor Author

@twada thx

@okoala
Copy link

okoala commented Apr 24, 2017

@twada good

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