Skip to content

Imported & Exported ES6 Namespaces now contained properties that would have been queuedExports #1422

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

Merged
merged 10 commits into from
Aug 5, 2020

Conversation

torch2424
Copy link
Contributor

closes #1417
relates to #1386
relates to #1415

This completely fixes how queuedExports are added to imported namespaces import * as ImportedNamespace. This creates a map of all importedNamespaces, and then as queued exports are resolve,d they are assigned to the appropriate namespace. It also ensures globals are correctly overriden when we finally generate the code for the said namespace.

And in this process I found serveral more bugs in general in just how namespaces are both imported and exported. And this fixes alllll of that 😄 🎉

@dcodeIO for the review 😄 👍

Screenshots of this working

Screenshot from 2020-07-31 17-11-40
Screenshot from 2020-07-31 17-03-58
Screenshot from 2020-07-31 17-03-42

@torch2424 torch2424 requested a review from dcodeIO August 1, 2020 00:23
@torch2424 torch2424 self-assigned this Aug 1, 2020
@torch2424 torch2424 changed the title Export star as with export from bug Imported & Exported ES6 Namespaces now contained properties that would have been queuedExports Aug 1, 2020
@dcodeIO
Copy link
Member

dcodeIO commented Aug 3, 2020

Thanks for investigating :) Added a few comments above, but I must admit that it's always a challenge to dive into this stuff because the linking is kinda abstract, so not sure how much sense the aliasNamespace suggestion makes. Perhaps you have a better idea how to make this more efficient without introducing new maps and somehow link sibling namespaces to make lookups cheap?

@torch2424
Copy link
Contributor Author

@dcodeIO

Thanks for investigating :)

You are welcome! I am thoroughly surprised I was able to figure this out actually haha 😂

Added a few comments above, but I must admit that it's always a challenge to dive into this stuff because the linking is kinda abstract, so not sure how much sense the aliasNamespace suggestion makes. Perhaps you have a better idea how to make this more efficient without introducing new maps and somehow link sibling namespaces to make lookups cheap?

So yes! I replied to your comments. Before I update the PR I'll wait to see what your replies are so I have a better idea on what to try to implement.

For the hasGlobal fix, If my comment was correct in that it's a set on our compiler class, than we should be good to go there.

For the Namespace set, yeah I just wanted you to either say I was understanding what you meant, and/or if my whole add functionality to the file class to allow adding exports later to a namespace makes sense.

Either way! Thanks for the review, super excited to get this in 😄

@torch2424
Copy link
Contributor Author

@dcodeIO Made Requested changes, and tests are passing on my end! This is good to go! 😸

Screenshot from 2020-08-04 11-57-53

@dcodeIO
Copy link
Member

dcodeIO commented Aug 4, 2020

Thanks! 👍 A few more comments above (sorry). I like how the mechanism works now!

@torch2424
Copy link
Contributor Author

Made requested changes @dcodeIO ! 😄 👍

Screenshot from 2020-08-04 18-28-47

@torch2424
Copy link
Contributor Author

Made requested changes! This is good to go @dcodeIO 😄 👍

Screenshot from 2020-08-05 09-22-12

@dcodeIO dcodeIO merged commit 5b910e3 into master Aug 5, 2020
@dcodeIO
Copy link
Member

dcodeIO commented Aug 5, 2020

Looks great, thank you!

@torch2424 torch2424 deleted the export-star-as-with-export-from-bug branch August 5, 2020 17:31
@torch2424
Copy link
Contributor Author

Yooooooooo! 😄 👍 Thanks for the review @dcodeIO 🚀

@github-actions
Copy link

github-actions bot commented Aug 6, 2020

🎉 This PR is included in version 0.14.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Import * as not working if the file has an export from
2 participants