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

Prevent rollup removing syntax error in try catch. #1946

Closed
LvChengbin opened this issue Feb 6, 2018 · 16 comments
Closed

Prevent rollup removing syntax error in try catch. #1946

LvChengbin opened this issue Feb 6, 2018 · 16 comments

Comments

@LvChengbin
Copy link

For some reasons, I need to create some error and catch the error message, and I have my code like this:

try { [ undeclared-variable ] } catch( e ) { console.log( e ) }

After I compiled the code with rollup, the [undeclared-variable] was removed by rollup, and I got the result:

$rollup test.js -f umd

test.js → stdout...
(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? factory() :
	typeof define === 'function' && define.amd ? define(factory) :
	(factory());
}(this, (function () { 'use strict';

try {  } catch( e ) { console.log( e ); }

})));
created stdout in 26ms

I know I can use throw new Error() instead, but is there any way to prevent rollup removing the code, or just ignore the line?

@lukastaegert
Copy link
Member

Admittedly, I was not expecting people to do that considering you can always do it safely this way:

if (typeof undeclaredVariable === 'undefined') console.log('Some error message');

To be more precise, we do not track access errors for undefined globals yet as the expectation is that this would noticeably decrease tree-shaking quality. However we could change that at some point if there is demand (and possibly add a flag to ignore it).

@LvChengbin
Copy link
Author

Because I need to get information from Error.stack, not just to print a log, so at least, for me, in this case, it would be better if rollup can allow me to ignore the error.

@guybedford
Copy link
Contributor

I tend to agree that this is a very rare pattern and typeof x is always the recommended form.

@LvChengbin what information would you need from the Error.stack here?

@LvChengbin
Copy link
Author

@guybedford in this case I encountered, I want to get the file list in trace information in the stack.
I don't mean I have to get the information in that way, but maybe it would be better if I can prevent rollup from correcting the code in some ways, such as using a comment like eslint-disable-line in eslint.

@kzc
Copy link
Contributor

kzc commented Feb 6, 2018

Should implement an option to prevent removal of undeclared globals as per previous discussion:

#1760 (comment)
#1726 (comment)

@kzc
Copy link
Contributor

kzc commented Feb 6, 2018

Generally there are two reasons for having undeclared globals:

  1. it's intentional
  2. it's a programmer error

The intentional case is demonstrated above. In the second case Rollup will obscure the problem.

Consider:

$ cat e.js
if (v) {}
const v = 1;
console.log("success?");

Expected:

$ cat e.js | node
[stdin]:1
if (v) {}
    ^
ReferenceError: v is not defined

Actual:

$ rollup e.js -f es --silent | node
success?

At the very least a warning would be useful.

@guybedford
Copy link
Contributor

When we can detect TDZ the above may make sense, but for now it seems like we're at a sensible compromise in terms of functionality.

@kzc
Copy link
Contributor

kzc commented Jun 7, 2018

When we can detect TDZ the above may make sense, but for now it seems like we're at a sensible compromise in terms of functionality.

So there's an acknowledgement of an unaddressed issue with undeclared globals (with or without try) but the ticket was closed anyway?

@guybedford
Copy link
Contributor

@kzc I don't see a way we can reasonably warn in these cases? TDZ seems unrelated to this issue, but we can certainly provide TDZ warnings along with the feature in future. But tracking TDZ warnings in this issue would likely not get picked up when that development happens unless it was its own explicit issue.

@kzc
Copy link
Contributor

kzc commented Jun 7, 2018

I may have hijacked this issue with a related problem. It seems that the unknown globals issue has not been addressed by Rollup, nor is it represented in an open issue:

#1726 (comment)

@guybedford
Copy link
Contributor

The thing is all globals are unknown... so in theory asdf shouldn't be tree-shaken.

If that is the suggestion then certainly let's reopen to alter that, but I'm not sure it makes sense for Rollup?

@kzc
Copy link
Contributor

kzc commented Jun 7, 2018

The thing is all globals are unknown...

Not all. We're talking about undeclared not-well-known globals.

asdf shouldn't be tree-shaken

Yes, that's my suggestion. Or at least have Rollup issue a warning.

but I'm not sure it makes sense for Rollup?

It would be useful to me and other Rollup users in my opinion, particularly to those of use who never run linters.

@lukastaegert
Copy link
Member

I think there are two different things at play here. TDZ errors are definitely very hard to catch for rollup in its current form but for globals, the prudent approach could be to track access to these variables as if it were a side-effect. One way of handling this without crippling tree-shaking could be to track global definitions and only treat access as a side-effect if there is no definition. This should help with the missing linter 😉. Would like to keep this open to track this (or move it to a new issue)

@shellscape
Copy link
Contributor

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

@lukastaegert
Copy link
Member

Actually this is fixed now anyway via the default try-catch deoptimization.

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

No branches or pull requests

5 participants