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

refactor!: convert to TypeScript #1171

Merged
merged 20 commits into from
Nov 7, 2023
Merged

Conversation

NiedziolkaMichal
Copy link
Member

This PR changes all .js files in editor/js to .ts and adds a minimum amount of necessary changes to remove basic errors, so it is easier to be reviewed.

  • Changes to .eslintrc.json were done because importing other modules was throwing an error. The same thing is done in refactor(lib): add types #1164
  • I changed back code async function () {}.constructor; to the version with Object.getPrototypeOf because error This expression is not constructable. Type 'Function' has no construct signatures. was being thrown.
  • I removed handleChoiceEvent because the replacement is easier to be understood and we don't need to rely on this.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@queengooborg
Copy link
Collaborator

Hey @NiedziolkaMichal! If you're down to rebase this, I'd be happy to get this merged shortly!

@caugner caugner changed the title refactor: Convert editor/js files to .ts refactor(editor/js): convert to TypeScript Oct 27, 2023
queengooborg
queengooborg previously approved these changes Oct 29, 2023
@queengooborg queengooborg dismissed their stale review October 29, 2023 18:10

Ack, looking at the test results, looks like a little more work is needed!

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@caugner
Copy link
Contributor

caugner commented Nov 2, 2023

@queengooborg There are some conflicts. Would you mind taking a look at those?

PS: I'm not sure, why the tests don't run on this PR. 😕

@queengooborg
Copy link
Collaborator

There are some conflicts. Would you mind taking a look at those?

They have been fixed now!

PS: I'm not sure, why the tests don't run on this PR. 😕

I think that was just a fluke; they're running on the latest commit it looks like.

@caugner caugner marked this pull request as draft November 3, 2023 18:40
@caugner
Copy link
Contributor

caugner commented Nov 3, 2023

@queengooborg Unfortunately, the tests are not passing yet. I'd be okay with @ts-ignoreing all those errors for now, and fix them in a separate PR.

@queengooborg queengooborg marked this pull request as ready for review November 5, 2023 04:11
Converting the Webpack config to TypeScript causes more issues than it solves.  Additionally, the Webpack documentation doesn't mention anything about using a .ts file for the configuration in its TypeScript guide.
@queengooborg
Copy link
Collaborator

Alright, so I've finished adding the remaining typedefs to the files and resolving TypeScript errors. I'm successfully able to run BoB and have had no problems with the interactive examples BoB hosts. I'm also successfully able to build and test BoB. However, it appears that building BoB from within interactive-examples isn't working, and I don't know why.

Copy link
Contributor

github-actions bot commented Nov 7, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@caugner
Copy link
Contributor

caugner commented Nov 7, 2023

@queengooborg It looks like 981875d did the trick, at least for running bob in the context of interactive-examples.

@caugner
Copy link
Contributor

caugner commented Nov 7, 2023

As for the failing puppeteer test, I can reproduce this locally on http://127.0.0.1:4444/pages/js/array-concat.html, it looks like the JS editor does not get initialized:

image

The DevTools don't show any console or network errors (apart from favicon.ico).

@queengooborg
Copy link
Collaborator

Hmm, I wasn't able to reproduce the issue locally until now -- I'll see if I can fix it!

@queengooborg
Copy link
Collaborator

Ah, figured out what it was and was able to fix it! I also added debugging logging to announce when such a case occurs again.

@caugner caugner changed the title refactor(editor/js): convert to TypeScript feat(editor/js): convert to TypeScript Nov 7, 2023
@caugner
Copy link
Contributor

caugner commented Nov 7, 2023

Awesome work, @queengooborg! 🙌

@caugner caugner merged commit e182364 into mdn:main Nov 7, 2023
6 checks passed
@queengooborg
Copy link
Collaborator

Thanks to @NiedziolkaMichal as well! Super excited for this to land!

@queengooborg queengooborg changed the title feat(editor/js): convert to TypeScript refactor!: convert to TypeScript Nov 7, 2023
@caugner
Copy link
Contributor

caugner commented Nov 7, 2023

Oh yes, sincere apologies, @NiedziolkaMichal indeed started this initiative, laying the most important groundwork and @queengooborg polished it and brought it over the finish line. Nice collaboration, glad to see this work accomplished! 🎉

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