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

export from generates wrong code #2214

Closed
chris-morgan opened this issue Mar 13, 2019 · 15 comments · Fixed by #6574
Closed

export from generates wrong code #2214

chris-morgan opened this issue Mar 13, 2019 · 15 comments · Fixed by #6574

Comments

@chris-morgan
Copy link
Contributor

<script>
export { a } from 'b';
</script>

That should yield the same result as this:

<script>
import { a } from 'b';
export { a };
</script>

But it doesn’t include the necessary import.

@Conduitry Conduitry added the bug label Mar 13, 2019
@Conduitry Conduitry added this to the 3.0 milestone Mar 13, 2019
@Conduitry
Copy link
Member

There are also some issues with the same code in a module script:

<script context='module'>
	export { a } from 'b';
</script>

In this case, the export could probably pass through unchanged in the compiled component. It is instead for some reason defining a as a read-only prop on the instance, referring to a global a which is not imported.

@Conduitry
Copy link
Member

I was just taking a look at this for real, and it is rather uglier than I was hoping. Some random thoughts:

In component.extract_imports we probably want to also extract out ExportNamedDeclaration nodes with a source present. Then in wrapModule (which is passed component.imports we can account for both types of nodes and generate the appropriate imports.

In component.extract_exports we also need to handle the case where we have an ExportNamedDeclaration with a source. Perhaps this can be handled in the same 'no declaration' case, but then we'd need to get each of the export ... from ... variables into component.var_lookup in some way. (There's also a TODO in there // TODO what happens with export { Math } or some other global? which seems to be an unpleasant case.) An alternative would be to have an entirely separate case before the else where we handle the presence of a source.

@Rich-Harris
Copy link
Member

I haven't looked at this specific issue yet but just wanted to flag up something I learned the hard way on Rollup — export { a } from './x.js' doesn't create a local binding to a:

export { a } from './x.js';

console.log(a); // ReferenceError: a is not defined

That may affect the code generation here, I'm not sure

@Conduitry
Copy link
Member

The export { Math }; thing is fixed in Acorn 6.0.7, so we won't have to worry about that if we upgrade, as the parser will complain right away about that. I am thinking though that for now we should just throw a compiler error if there are any export froms and we can deal with properly handling that at another time.

Rich-Harris added a commit that referenced this issue Mar 15, 2019
Disallow `export ... from` statements
@Conduitry Conduitry modified the milestones: 3.0, 3.x Mar 16, 2019
@Conduitry
Copy link
Member

Beta 16 refuses to compile code with export ... froms. Gonna abuse issues slightly and switch this one from a bug to an enhancement, and use it later for adding support for this.

@TheComputerM

This comment has been minimized.

@stale
Copy link

stale bot commented Jun 27, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 27, 2021
@chris-morgan
Copy link
Contributor Author

No, no, no, no, no, please turn the @Stale bot off. It’s a menace that does almost nothing good and causes significant harm. Everyone hates it.

@stale stale bot removed the stale-bot label Jun 27, 2021
@pngwn
Copy link
Member

pngwn commented Jun 28, 2021

We are drowning, i don't doubt you dislike it but if we want any hope of prioritising important issues we need to automate some stuff. As you can see, a simple comment dismisses it.

@arxpoetica
Copy link
Member

It’s a menace that does almost nothing good and causes significant harm.

Actually, it brought this ticket back into a semi-conscious state. I'd call that good.

@chris-morgan
Copy link
Contributor Author

chris-morgan commented Jun 28, 2021

I have never seen the @Stale bot or any directly equivalent to it achieve a net positive outcome. Never. It results in disgruntled people, extra expenditure of effort (for reporters and maintainers), real stuff getting lost when people get fed up with poking the bot (I have no intention of poking it further), and more extensive filing of duplicates.

You say a simple comment dismisses it, but it doesn’t—it only does this time. Next time, it continues to annoy.

This is an issue tracker. Use labels, projects, milestones and the likes for prioritising stuff. Not sweeping stuff under the carpet.

@pngwn
Copy link
Member

pngwn commented Jun 28, 2021

It is an issue tracker but we don't have a backlog, or planning sessions, or a project board. Or the resources to even triage and tag effectively.

If it is important someone will respond / reopen, popular issues are exempt from the bot, we can't fix everything and this is pretty much our only view on stuff that need to be addressed. We need to make some attempt to make sure that everything is still relevant and reduce the noise to a degree where we can actually manage it.

I understand the trade-offs with stale bots but we don't have many options. I appreciate your experiences but that doesn't make them a fact. We have discussed this internally and this is what we are doing. If you have any other actionable alternatives outside of saying the bot is bad then we are all ears.

@chris-morgan
Copy link
Contributor Author

Closing issues doesn’t solve anything. Closing issues in GitHub just sweeps them under the carpet and helps everyone to forget about them, which is just not what you want—the fact that GitHub search excludes closed items by default is a large part of the problem with it.

As applied to software projects with general-purpose issue trackers, the @Stale bot is fundamentally phenomenally bad idea, a road paved with good intentions.

I presented an actionable alternative: labels. Possibly automatically applied, but it’s certainly better to spend a little bit of time on manual triage. It honestly doesn’t take long to skim through a few hundred issues and bin them into labels.

609 open issues? That’s honestly not a problem. Not at all. There’s nothing wrong with having a large number of issues open, if they do correspond to real things—even things that you may not expect to get to for years, if ever, because that might change or someone might decide they want to deal with one.

Closing issues that aren’t dealt with is bad. Please don’t do it.

@pngwn
Copy link
Member

pngwn commented Jun 28, 2021

You make valid points but we'll need to figure out a pragmatic way to do it. We will think on it. Most issues have been manually labelled as stale rather than automated and closure will be manual too, so we have time to think.

This is the wrong place for this conversation though.

@Conduitry
Copy link
Member

This is supported now in 3.41.0.

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

Successfully merging a pull request may close this issue.

6 participants