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

Keep resolving queued imports while there is progress #1415

Merged
merged 2 commits into from
Jul 26, 2020
Merged

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Jul 25, 2020

Potentially fixes #1386. Looks like this is an ordering issue where imports depend upon other imports to be resolved first. In the specific case

// index.ts
import { one } from "./two"; // 1: is attempted first and...
one.one;
// two.ts
import * as one from "./one"; // 3: has not been processed yet, hence fails
export { one }; // 2: is found, but...
// one.ts
export var one = 1;

Instead of giving up early, the algorithm now repeats while it makes progress.

  • I've read the contributing guidelines

@torch2424 Can you confirm that this fixes the issue?

Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

From #1386

Hmm, what should happen for star imports is that the file becomes converted to a namespace via File#asImportedNamespace, and lookups are then performed against that namespace. Perhaps there's something wrong when such an imported namespace is populated or goes through another layer of exports (in wrong order)?

Ah! Yeah totally, I had a feeling this may be an ordering issue, but I didn't suggest that as again, don't have a complete grasp on how things were working. But glad the fix wasn't too bad, and more that one import needs to wait for others haha! 😄 Also, Hope that I didn't push you to make the fix, was totally happy to do it as well, but as you could tell, I was just very lost haha 😂


@torch2424 Can you confirm that this fixes the issue?

So I can't share the code (yet) for the use case I am using it for, but I can tell you in the library I'm developing for work, it works perfectly! Thanks @dcodeIO ! 😄

And everything looks good, so if you wanted my review, approving! 😄 👍

@@ -5,3 +5,6 @@

declare const global: Record<string,unknown>;
declare function require(name: string): unknown;
declare namespace console {
function log(...args: unknown[]): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for adding this for console log debuggers like me 😂

}
}
}
if (!madeProgress) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhhhh! I had a feeling it may have been an ordering issue where one import had to resolve before others 😄 👍

@@ -6,3 +6,6 @@ export {
default,
default as renamed_default
} from "./reexport";

import { exportstar } from "./reexport";
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding a test as well! 👍

@dcodeIO dcodeIO merged commit 348c42b into master Jul 26, 2020
@github-actions
Copy link

🎉 This PR is included in version 0.14.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@torch2424
Copy link
Contributor

Yooooo thanks for merging / doing this @dcodeIO ! 😄 👍

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

Successfully merging this pull request may close these issues.

"No exported member" when Importing an exported "import * as"
2 participants