Skip to content

Add module id when warning about top level this - #1012

Merged
Rich-Harris merged 3 commits into
rollup:masterfrom
danielkcz:FredyC-patch-1
Oct 9, 2016
Merged

Add module id when warning about top level this#1012
Rich-Harris merged 3 commits into
rollup:masterfrom
danielkcz:FredyC-patch-1

Conversation

@danielkcz

Copy link
Copy Markdown
Contributor

When during a bundling I get a cryptic message stating The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten, it's not very helpful to find actual problematic module especially since it can occur anywhere inside node_modules.

Adding module ID to the message should help locate and possibly remove the problem.

Hard to fix the problem when not knowing which module actually has it.
@Victorystick

Copy link
Copy Markdown
Contributor

Good idea! Could you include the line number as well?

@danielkcz

danielkcz commented Oct 3, 2016

Copy link
Copy Markdown
Contributor Author

I've fixed tests. Did not really expect there will be one checking exact format of the message :) Had to do it bit differently instead of checking for a fixed message because the path to the file is dynamic. I hope it's enough like that.

@Victorystick That would be lovely. I tried looking at what data are available there, but could not find information about line number there :(

@Victorystick

Copy link
Copy Markdown
Contributor

To get the line and column location of an AST node just use utils/getLocation! 👍

@danielkcz

danielkcz commented Oct 4, 2016

Copy link
Copy Markdown
Contributor Author

@Victorystick Ok I did try it like this, but I don't think it's correct. It returns line 4 in test, but it should be line 3. I am not sure what other number then this.module.ast.end use.

image

this.alias = this.module.bundle.context;
if ( this.alias === 'undefined' ) {
this.module.bundle.onwarn( `The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten (in ${this.module.id})` );
const location = getLocation(this.module.code, this.module.ast.end);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All AST nodes have start and end properties. Use this.start instead of this.module.ast.end. (That'll always give you the last line of the bundle.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't seem about right because the start seems to be always zero in the test. Even if the actual problem occurred one line 4.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try now using either this.start or this.module.start, both are undefined.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Victorystick can you please advice what else I should try?

@Rich-Harris
Rich-Harris merged commit b164a46 into rollup:master Oct 9, 2016
@Rich-Harris

Copy link
Copy Markdown
Contributor

Thanks! I fixed the location thing, and tweaked the message a bit so that it includes a link to https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined.

Had to do it bit differently instead of checking for a fixed message because the path to the file is dynamic

For #1033 I added a utility, relativeId, that takes a module ID and makes it relative to the CWD if possible, which fixes that problem and makes the message more readable – I've incorporated it here.

@Rich-Harris

Copy link
Copy Markdown
Contributor

Released as 0.36.2

@danielkcz

Copy link
Copy Markdown
Contributor Author

Thanks @Rich-Harris. So you think that using module.ast.end is correct to find line number? It doesn't work that well as test file apparently has issue at a different spot than the one recovered by this. Might need some tweaking.

@danielkcz
danielkcz deleted the FredyC-patch-1 branch October 9, 2016 20:16
@Rich-Harris

Copy link
Copy Markdown
Contributor

So you think that using module.ast.end is correct to find line number?

No, I changed it to use this.start4040022

@danielkcz

Copy link
Copy Markdown
Contributor Author

Ah I've missed that. Looks great, amazing.

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