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

Typescript in svelte in builder #15063

Merged
merged 13 commits into from
Nov 26, 2024

Conversation

adrinr
Copy link
Collaborator

@adrinr adrinr commented Nov 25, 2024

Description

Enabling typescript on svelte in the builder. This PR only sets the right configuration for enabling using typescript in svelte, making use of it in a file in the builder and one in bbui.
This PR will be followed by another enabling using typescript instead of js, and further refactors of more files.
Some of the complex types had been just mapped to any to keep this one simple. Better typing across the stack will follow.

Launchcontrol

Enable ts in svelte in the builder

Feature branch env

Feature Branch Link

Copy link

linear bot commented Nov 25, 2024

Copy link

qa-wolf bot commented Nov 25, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@github-actions github-actions bot added firestorm Data/Infra/Revenue Team size/s labels Nov 25, 2024
@adrinr adrinr added feature-branch Release this PR code into a feature branch and removed size/s labels Nov 25, 2024
@adrinr adrinr changed the title [WIP] Typescript in builder Typescript in svelte in builder Nov 26, 2024
@adrinr adrinr marked this pull request as ready for review November 26, 2024 09:57
Comment on lines -5 to -10
export let value = null
export let disabled = false
export let readonly = false
export let error = null
export let appendTo = undefined
export let ignoreTimezones = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. And typescript complained about that 😄

Copy link
Member

Choose a reason for hiding this comment

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

I've actually removed this file altogether in my branch. BBUI is consumed by frontend-core so I saw the same issues, and this file is actually entirely unused. You can leave it in though - I can deal with the small conflict.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the feeling that it was not used at all. I just did not want to remove it just in case some old app was using it somehow 🪄

@@ -18,8 +18,6 @@
export let palette
export let c1, c2, c3, c4, c5

// Area specific props
export let area
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused?

Copy link
Collaborator

@mike12345567 mike12345567 left a comment

Choose a reason for hiding this comment

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

LGTM - very nice, didn't require much configuration change!

@adrinr adrinr requested a review from aptkingston November 26, 2024 11:16
@adrinr adrinr enabled auto-merge November 26, 2024 11:22
@adrinr adrinr disabled auto-merge November 26, 2024 11:24
@adrinr adrinr merged commit 9fd3b38 into master Nov 26, 2024
19 checks passed
@adrinr adrinr deleted the BUDI-8866/builder-vite-tooling-svelte-typescript branch November 26, 2024 12:28
@github-actions github-actions bot locked and limited conversation to collaborators Nov 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-branch Release this PR code into a feature branch firestorm Data/Infra/Revenue Team size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants