Skip to content

feat(create-vite): add solidjs templates (#12218)#12241

Merged
bluwy merged 48 commits into
vitejs:mainfrom
AbdelrahmanDwedar:solidjs-template
Jul 3, 2023
Merged

feat(create-vite): add solidjs templates (#12218)#12241
bluwy merged 48 commits into
vitejs:mainfrom
AbdelrahmanDwedar:solidjs-template

Conversation

@AbdelrahmanDwedar
Copy link
Copy Markdown
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.

@bolt-new-by-stackblitz
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

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

Thank you for the review 👍🏼👍🏼

I'll add this changes tonight hopefully.

@AbdelrahmanDwedar
Copy link
Copy Markdown
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 AbdelrahmanDwedar requested a review from bluwy March 2, 2023 19:36
@AbdelrahmanDwedar
Copy link
Copy Markdown
Contributor Author

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

@hanneswidrig
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor Author

AbdelrahmanDwedar commented Apr 4, 2023

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Any updates?

@ArnaudBarre
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@hanneswidrig
Copy link
Copy Markdown

@AbdelrahmanDwedar
Copy link
Copy Markdown
Contributor Author

"jsxImportSource": "solid-js"

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

@AbdelrahmanDwedar
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
xinxinhe1810 pushed a commit to xinxinhe1810/vite that referenced this pull request Jul 4, 2023
Co-authored-by: Arnaud Barré <arnaud.barre72@gmail.com>
Co-authored-by: bluwy <bjornlu.dev@gmail.com>
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