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.

Sign up for GitHub

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

Jump to bottom

worker: fix faulty import type #1072

Merged
merged 1 commit into from Jul 2, 2021
Merged

worker: fix faulty import type #1072

merged 1 commit into from Jul 2, 2021

Conversation

samouri
Copy link
Member

@samouri samouri commented Jul 2, 2021 •
edited

summary
Recent PR #1067 had an error. The worker-thread index.ts and index.amp.ts should not use import type for imports whose values are actually used!

I'm incredibly surprised that TypeScript didn't catch this, and opened up an issue: microsoft/TypeScript#44868

testing done

Manually ran yarn demo and checked each example

@samouri samouri self-assigned this Jul 2, 2021
@samouri samouri marked this pull request as ready for review July 2, 2021 14:09
@kristoferbaxter kristoferbaxter merged commit 35139ef into main Jul 2, 2021
@kristoferbaxter kristoferbaxter deleted the myb branch July 2, 2021 17:52
samouri added a commit that referenced this pull request Jul 2, 2021
@samouri
Revert "worker: fix faulty import type (#1072)"
df50202 
This reverts commit 35139ef.
@samouri samouri mentioned this pull request Jul 2, 2021
Merged
@samouri
Copy link
Member Author

samouri commented Jul 2, 2021 •
edited

This wasn't ready yet! I had planned on reverting all of the type imports since while I didn't spot any issues in yarn demo, there could still be weird bugs lurking. Followup in #1073

samouri added a commit that referenced this pull request Jul 2, 2021
@samouri
Remove all "import type" after discovery of a TS bug (#1073)
95bb805 
* Revert "worker: fix faulty import type (#1072)"

This reverts commit 35139ef.

* Revert "Separate out all type-only imports (#1067)"

This reverts commit b1f1658.
@samouri samouri mentioned this pull request Jul 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristoferbaxter kristoferbaxter kristoferbaxter approved these changes

Assignees

@samouri samouri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@samouri @kristoferbaxter