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(create-vite): add solidjs templates (#12218) #12241

Merged
merged 48 commits into from
Jul 3, 2023

Conversation

AbdelrahmanDwedar
Copy link
Contributor

@AbdelrahmanDwedar AbdelrahmanDwedar commented Feb 28, 2023

Description

Adding the templates for solid.js to the creating templates.

  • Added the javascript template
  • Added the typescript template
  • Added the template to the list of frameworks that will be shown

Additional context

This pull request closes: #12218


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented Feb 28, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@AbdelrahmanDwedar AbdelrahmanDwedar changed the title [Feat] Adding Solidjs framework (#12218) [Feature / cli] Adding Solidjs framework (#12218) Feb 28, 2023
@AbdelrahmanDwedar AbdelrahmanDwedar changed the title [Feature / cli] Adding Solidjs framework (#12218) feat: Add Solidjs framework (#12218) Feb 28, 2023
@AbdelrahmanDwedar AbdelrahmanDwedar changed the title feat: Add Solidjs framework (#12218) feat: add Solidjs framework (#12218) Feb 28, 2023
@AbdelrahmanDwedar
Copy link
Contributor Author

The failing test must be because this is a template and it uses a dependencies that we don't need in vite itself, I don't know if we should be looking for it in some way.

Can someone check it out please. 😁

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

It needs a bit of changes to match currently the other templates. To fix the lint, you need to follow the template naming convention of template-*. For solid, it should be template-solid and template-solid-ts.

The comments below are for the JS template, but they should also apply for the TS template. You can also follow https://github.com/bluwy/create-vite-extra/tree/master/template-ssr-solid and https://github.com/bluwy/create-vite-extra/tree/master/template-ssr-solid-ts to see how the base code is setup.

packages/create-vite/solid-js/_gitignore Outdated Show resolved Hide resolved
packages/create-vite/solid-js/index.html Outdated Show resolved Hide resolved
packages/create-vite/solid-js/package.json Outdated Show resolved Hide resolved
packages/create-vite/solid-js/src/App.jsx Outdated Show resolved Hide resolved
packages/create-vite/solid-js/src/App.jsx Outdated Show resolved Hide resolved
packages/create-vite/solid-js/vite.config.js Outdated Show resolved Hide resolved
packages/create-vite/solid-js/vite.config.js Outdated Show resolved Hide resolved
@AbdelrahmanDwedar
Copy link
Contributor Author

Thank you for the review 👍🏼👍🏼

I'll add this changes tonight hopefully.

@AbdelrahmanDwedar
Copy link
Contributor Author

AbdelrahmanDwedar commented Mar 1, 2023

This should do for the changes you requested.

For the failing test, do you have any thoughts about the reason and any suggestions to solve it?

@AbdelrahmanDwedar
Copy link
Contributor Author

Any updates for the issue? Can someone recommend anything to try to solve this check?

@hanneswidrig
Copy link

hanneswidrig commented Apr 1, 2023

Try rebasing onto the latest commits in main, it does look like it was working until you simplified the vite.config.{ts,js}.

I also think you might want to stick to the official templates https://github.com/solidjs/templates. They power https://solid.new.

@AbdelrahmanDwedar
Copy link
Contributor Author

Try rebasing onto the latest commits in main, it does look like it was working until you simplified the vite.config.{ts,js}.

I also think you might want to stick to the official templates https://github.com/solidjs/templates. They power https://solid.new.

I just did now, waiting to see if there are any changes, but for the changes to the vite.config.{ts, js} I just tried to do them exactly the same in the https://solid.new/ and they just worked perfectly. The rest of the template is inherited from the style that all the other create-vite templates had, just changed the looks to meet that the rest is mostly the same as the template of solidjs, tell me if you noticed differences I didn't. 😄

But thanks for helping.

@hanneswidrig
Copy link

I made some additional changes to add the links to the website, also the build seems to be passing on my branch. #12726

@AbdelrahmanDwedar
Copy link
Contributor Author

AbdelrahmanDwedar commented Apr 4, 2023

I just had to rebase my branch... Didn't think of that one heh...

Copy link
Member

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

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

Few changes required:

  • let's remove the spinning logo. It works well for React but less for solid
  • make tsc works on the TS template. It requires adding vite-env.d.ts and fixing the JSX typing issue.

@AbdelrahmanDwedar
Copy link
Contributor Author

Any updates?

@ArnaudBarre
Copy link
Member

I need to test that there is no tsc issues now (didn't see changes related to the jsx target issue)

@AbdelrahmanDwedar
Copy link
Contributor Author

I'm not sure how to fix the typing issues with jsx yet, I'll research it

@hanneswidrig
Copy link

@AbdelrahmanDwedar
Copy link
Contributor Author

"jsxImportSource": "solid-js"

I added it and it's still giving me errors in vscode

@AbdelrahmanDwedar
Copy link
Contributor Author

This should make the code run and it works in the browser.

But still there's an issue with the vscode recognizing the code some how.

App.tsx:
image

tsconfig.json:
image

@ArnaudBarre
Copy link
Member

Yeah stackblitz is missing the latest update of VSCode that fix this. Let's wait a few days and test again

@AbdelrahmanDwedar
Copy link
Contributor Author

This should make the code run and it works in the browser.

But still there's an issue with the vscode recognizing the code some how.

App.tsx: image

tsconfig.json: image

Just updated vscode and the errors disappeared

@AbdelrahmanDwedar
Copy link
Contributor Author

The tests passes!!

Can someone now review again and see if there's any out-dated things we need to fix or anything?

@ArnaudBarre ArnaudBarre added this to the 4.4 milestone Jun 3, 2023
ArnaudBarre
ArnaudBarre previously approved these changes Jun 16, 2023
@AbdelrahmanDwedar
Copy link
Contributor Author

Is everything going fine? This PR was approved about two weeks ago and no updates then..

@oarsheo

This comment was marked as spam.

@bluwy bluwy merged commit 277e844 into vitejs:main Jul 3, 2023
13 checks passed
xinxinhe1810 pushed a commit to xinxinhe1810/vite that referenced this pull request Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Feature / cli] Add template / option for solidjs in cli
6 participants