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

update missing options.target from dev error (#753) #754

Closed
wants to merge 1 commit into from

Conversation

Rich-Harris
Copy link
Member

Ref #753. It turns out we're already throwing an error if options.target is missing, but only in dev mode.

So I'm in two minds about whether to upgrade it. One the one hand, this is exactly what dev mode is for — adding these kinds of errors without bulking out generated code in production. On the other, you can end up with some fairly surprising bugs if you're not in dev mode.

Interested to hear what people think.

@Conduitry
Copy link
Member

As someone who's never been burned by this behavior in prod mode, I feel like this should stay a dev-mode-only error. As far as I can tell, when dev mode's off, we don't currently generate any throw new Errors in the code. That seems like a reasonable guideline to follow, and I don't know why this one check would get special treatment.

@saibotsivad
Copy link
Contributor

I hadn't actually noticed the dev mode option until now (I've got to go study the docs apparently), and I was surprised by the number of warnings in our app when I turned it on. Still, I would tend to agree with @Conduitry at the moment.

@Rich-Harris
Copy link
Member Author

I've got to go study the docs apparently

about that... 😬

@Rich-Harris
Copy link
Member Author

cool. closing this

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.

3 participants