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

feat(cta): add SolidJS recipe #2619

Merged
merged 19 commits into from
Sep 22, 2021
Merged

feat(cta): add SolidJS recipe #2619

merged 19 commits into from
Sep 22, 2021

Conversation

davedbase
Copy link
Contributor

@davedbase davedbase commented Sep 21, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Docs
  • New Binding Issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@davedbase davedbase requested a review from a team as a code owner September 21, 2021 01:18
Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

Thanks for your efforts, there is a couple of things to change (I think you copied the recipe from dev branch instead of next which has these fixes already).

For tests, you can add it here https://github.com/tauri-apps/tauri/blob/next/tooling/create-tauri-app/test/spawn.test.mjs#L27 and here https://github.com/tauri-apps/tauri/blob/next/.github/workflows/test-cta.yml#L8

tooling/create-tauri-app/src/recipes/solid.ts Outdated Show resolved Hide resolved
tooling/create-tauri-app/src/recipes/solid.ts Outdated Show resolved Hide resolved
tooling/create-tauri-app/src/recipes/solid.ts Outdated Show resolved Hide resolved
@amrbashir
Copy link
Member

I almost forgot, the PR is missing a changefile and please fill out the PR template instead of removing it.

@davedbase davedbase requested a review from a team September 21, 2021 11:31
@davedbase
Copy link
Contributor Author

My apologies @amrbashir I threw this up too quickly then rushed away to another task without reading the procedure properly. I've changed the description with the proper PR template, changefile (I looked at Svelte's changefile) and applied the changes you requested.

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

No need to apologize.

Just some more small changes and this PR is ready to go.

.github/workflows/test-cta.yml Show resolved Hide resolved
tooling/create-tauri-app/src/recipes/solid.ts Outdated Show resolved Hide resolved
Co-authored-by: Amr Bashir <48618675+amrbashir@users.noreply.github.com>
Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

Sorry I forgot about these.

tooling/create-tauri-app/src/recipes/solid.ts Outdated Show resolved Hide resolved
tooling/create-tauri-app/src/recipes/solid.ts Outdated Show resolved Hide resolved
@davedbase
Copy link
Contributor Author

@amrbashir my apologies this PR could have gone a lot smoother. I hope things are looking much better now!

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

yeah the PR looks good to me now but since we are in a feature-freeze phase, it might be a while before it gets merged.

.github/workflows/test-cta.yml Outdated Show resolved Hide resolved
@davedbase
Copy link
Contributor Author

Yup no worries! Looking forward to when its merged.

@lucasfernog lucasfernog changed the title WIP: Add SolidJS recipe feat(cta): add SolidJS recipe Sep 21, 2021
@davedbase
Copy link
Contributor Author

@amrbashir should I be concerned that it's not passing the eslint test? I adjusted the types to mitigate it but it's still stumbling....

@amrbashir
Copy link
Member

@amrbashir should I be concerned that it's not passing the eslint test? I adjusted the types to mitigate it but it's still stumbling....

That is actually a formatting issue, should be fixed by running yarn format.

@davedbase
Copy link
Contributor Author

@amrbashir the test runner didn't list any formatting errors. When I ran prettier it didn't change much except removed end of line. shrugs lol

@lucasfernog lucasfernog merged commit 71ea86a into tauri-apps:next Sep 22, 2021
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.

None yet

3 participants