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

Convert most of the remaining code to Typescript #1312

Closed
wants to merge 4 commits into from

Conversation

Krisztiaan
Copy link
Contributor

@Krisztiaan Krisztiaan commented Oct 8, 2020

Except CLI.

#1291 superset

Sorry about the rule against src/* but typescript does not change import paths and does not plan to do so either, so declaration files would come out broken after the build process.

Simplified routing, I think it, along with auth, could use some planning to make it more concise.

@thedavidprice
Copy link
Contributor

🤯

dude

This is amazing. And not taken for granted! I have some ideas for the next steps here and need to coordinate with current status (e.g. Peter is on vacation this week). Looping back a bit later with specifics to keep this momentum.

🚀🚀*


  • the coveted double rocket for when I get really excited

@peterp
Copy link
Contributor

peterp commented Oct 10, 2020

Wow awesome! 💥

We already had packages/web conversion in progress, but we had not yet converted the FlashComponent and it looks like you've done that last step!

Could we break this pull-request up into a few chunks so that they're easier to discuss and review?

  • Rebase with main
  • Create a PR to merge/ review FlashComponent + config.js
  • Create a PR for simplified auth
  • Create a PR for simplified routing

For the auth + routing discussion maybe we could jump on a call and chat about it?

@peterp peterp mentioned this pull request Oct 10, 2020
8 tasks
@Krisztiaan
Copy link
Contributor Author

There are some points that'd be nice to discuss, I'm available over on Discord, @Krisztiaan#9999.

I am aware that the PR is way too complicated for proper merging and review, but I figured it'd be worth making a draft anyways to kick things off. We can always "remake" it properly.

It also helped me a lot in trying to understand how routing and auth works currently, and from the difficulty of typing, to the presence of some magic, a few ideas popped up that'd be worth discussing especially in the context of sides support.

For starters, the first proposal I've made is available on the RW forum, [Proposal] Don’t magically wrap auth providers

@thedavidprice
Copy link
Contributor

@Krisztiaan trying to keep momentum here as Peter's just back from 10 days vacation and focused on a lot of things this week (including some current bugs in main that are in the way of v0.20.0 release).

Given his suggestion above, might it be possible to move forward with "Create a PR to merge/ review FlashComponent + config.js" this week? Given some help from Kim-Adeline as well (who's been working on Web TS support), that seems possible to get merged quickly.

And then you and Peter could schedule a time for the discussion around Auth and Routing.

Thoughts/suggestions otherwise?

Thanks again!

@Krisztiaan
Copy link
Contributor Author

I'll try and find some time for it this week.

@thedavidprice
Copy link
Contributor

Thanks @Krisztiaan 100% understood and no pressure intended. Just keep us posted and I'll do the same.

@Krisztiaan
Copy link
Contributor Author

I'm still here, just didn't have the time to do this. I see however that progress is being made outside of this PR too, nice!

@Tobbe
Copy link
Member

Tobbe commented Oct 19, 2020

@Krisztiaan Impressively amount of files touched in this PR! I'd hate for all that effort going to waste. Do you think you could rebase this on top of all the latest changes in the repo?

@Krisztiaan
Copy link
Contributor Author

@Tobbe that's the plan

@papaponmx
Copy link
Contributor

Is this still a work in progress?

@Krisztiaan
Copy link
Contributor Author

I think it's more like "discontinued" due to scope, one PR breaking it up was made after this, #1382, but no contrib. activity, and other projects on my end kind of pushed it to the back for me. This PR can be closed, as it's way too old and overarching to still be relevant, as far as I can tell.

@Tobbe
Copy link
Member

Tobbe commented Feb 14, 2021

Closing as requested

@Tobbe Tobbe closed this Feb 14, 2021
@thedavidprice
Copy link
Contributor

Hi @papaponmx and @Krisztiaan We're still very much in need of help in the TS Support side of things. Please DM me here if you're interested in learning more. No pressure of course!

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.

5 participants