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

add TypeScript support at project init #41

Merged
merged 7 commits into from
Oct 27, 2020
Merged

Conversation

Rich-Harris
Copy link
Member

One possible approach to #31 — adds an init-time prompt that asks if you want to add TypeScript, and if so adds it to package.json and edits the example component to use lang="ts".

It also adds svelte-preprocess by default, which I'm not totally sold on (since it's an extra moving part), but i'm probably in the minority there. (Should svelte-preprocess be moved into this repo? It feels like it probably belongs)

@benmccann
Copy link
Member

This PR lgtm.

Is the soft-bats-build.md some auto-generated thing and you always end up with random names like that?

I'd probably lean away from having svelte-preprocess be part of this repo, but @kaisermann would be the real person to ask

@Conduitry
Copy link
Member

Yeah the random filenames are part of https://github.com/atlassian/changesets

@kaisermann
Copy link
Member

I've been asking myself if svelte-preprocess shouldn't live with the rest of the language-tools for quite a while, but now with kit in the mix, it probably makes more sense to move it here since it fits the Everything you need to build a Svelte app. motto. The only downside I'm seeing now is having a bunch of possibly unrelated issues polluting the issues list. When you say "an extra moving part" you mean purely from a repository/maintainership perspective?

@Conduitry
Copy link
Member

I read the extra moving part as referring to including it as something to be installed with the template, regardless of which repo the code lived in.

It would probably be nice if package.json could be varied depending on whether the user chose to init their app in TS mode.

@kaisermann
Copy link
Member

Completely agree with the varied package.json. Even if lots of people use Svelte with some kind of preprocessor, we shouldn't install it if it's not necessary. We're already modifying the package.json with the add_typescript, couldn't we have an add_svelte_preprocess method? This could also help if we decide to have a prompt for preprocessors at some point.

@dominikg
Copy link
Member

some discussion about an official typescript template took place here sveltejs/svelte#5016, may still be relevant?

In svite i started with a small script too but ultimately went with degit and only slight modifications to package.json because i think it can be helpful to have the template laid out in a git repo so it is accessible outside of a created project on disc (eg linking for documentation, discussions etc).

But that can be solved with some form of automation that updates an example repo every time the base template or a modification changes.

And i really like the concept of modifications! Even more so if they turned out to be "standalone" and accessible via cli later.
console.log(`You can add TypeScript support later with ${bold(cyan('npm install -D typescript'))}`); would then be sth. like console.log(`You can add TypeScript support later with ${bold(cyan('svelte add typescript'))}`);

Maybe i missed some things but should this modification add a tsconfig.json like (see https://github.com/sveltejs/template/blob/431bd4d58e59b46ebfa1f4fc2c1ab55853fc1521/scripts/setupTypeScript.js#L90)
and turn all .js files into .ts?

Other ideas for modifications: add svelte-check , add vscode extension recommendation.

@benmccann
Copy link
Member

Here's the one for Sapper (which is rather recent and a bit more involved): https://github.com/sveltejs/sapper-template/blob/master/scripts/setupTypeScript.js

@dummdidumm
Copy link
Member

Answering @dominikg

And i really like the concept of modifications! Even more so if they turned out to be "standalone" and accessible via cli later.
console.log(`You can add TypeScript support later with ${bold(cyan('npm install -D typescript'))}`); would then be sth. like console.log(`You can add TypeScript support later with ${bold(cyan('svelte add typescript'))}`);

Isn't that what this prompt also does? It relies on some initial standalone template as far as I can see and then applies modifications to it.

Maybe i missed some things but should this modification add a tsconfig.json like (see https://github.com/sveltejs/template/blob/431bd4d58e59b46ebfa1f4fc2c1ab55853fc1521/scripts/setupTypeScript.js#L90)
and turn all .js files into .ts?

Yes, that's missing from this PR, should be added.

Other ideas for modifications: add svelte-check , add vscode extension recommendation.

  • svelte-check: Nice idea for a separate step!
  • vscode extension: I think it would be better to have some kind of message in the end of the setup like "Here are some IDE recommendations: ..."

Answering @kaisermann

Completely agree with the varied package.json. Even if lots of people use Svelte with some kind of preprocessor, we shouldn't install it if it's not necessary. We're already modifying the package.json with the add_typescript, couldn't we have an add_svelte_preprocess method? This could also help if we decide to have a prompt for preprocessors at some point.

I think it would be better to hide this behind more semantic flags/prompts like "add typescript?", "what styling solution do you want to use? (css, scss, less, other)", and depending on the inputs of the user svelte-preprocess will be added as well as a corresponding entry to svelte.config.js.

@kaisermann
Copy link
Member

kaisermann commented Oct 22, 2020

Yeah, what I meant was to at least have a method to do it on a need-basis, preventing the lib to be hardcoded in the package.json ✌️ . Semantic options would indeed be DX friendly and I think we should cover just the basic cases like scss, stylus, etc. These don't require any configuration, unlike postcss and babel, which could make these prompts too complicated.

@dominikg
Copy link
Member

Isn't that what this prompt also does? It relies on some initial standalone template as far as I can see and then applies modifications to it.

Yes, but as is this modification only works for the vanilla template (eg assumes Counter.svelte exists). What i meant with standalone was that it could be called later-later, when the user already started working on the project and now feels a need to add typescript.

@dummdidumm
Copy link
Member

Isn't that what this prompt also does? It relies on some initial standalone template as far as I can see and then applies modifications to it.

Yes, but as is this modification only works for the vanilla template (eg assumes Counter.svelte exists). What i meant with standalone was that it could be called later-later, when the user already started working on the project and now feels a need to add typescript.

Ah, I get it now, thanks. Maybe we can get the best of both worlds and run certain modifications only on creation. So there would be a common subset, and npm init svelte would add additional modifications on top.

@Rich-Harris
Copy link
Member Author

my thoughts:

  • we want to keep modifications to a bare minimum — they're inherently brittle (if you have several, and some of them can do things like editing Counter.svelte, then you end up having a hard time ensuring that any combination of modifications will result in a working app)
  • i like the of only installing svelte-preprocess if you say yes to TypeScript
  • i'm just going to say it: TypeScript is a good addition to a codebase, Stylus etc are not IMHO. i think it makes sense for us to make TS support as seamless as possible but in general i think there should be a little bit of friction if you want to start using non-standard languages. (the distinction is subjective, but we endorse TS through our own use of it in the svelte codebase, and on the basis of adoption TS basically is a standard langauge). therefore, i think it'd be fine to not ask about those other languages or about svelte-preprocess outside the TS prompt
  • i'm also wary of post-creation scripts. the challenge of making robust modifications is exponentially harder when you have no idea what chaos has been done to the project. we should strive for an architecture that makes it easy for people to make those modifications themselves and understand what they're doing — teach a man to fish — rather than npx svelte give-me-a-fish. if the CLI turns into 'Clippy, but for Svelte apps' we will regret it

looking into the tsconfig.json thing, good catch

Copy link

@orta orta left a comment

Choose a reason for hiding this comment

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

concept seems good to me 👍🏻

@dominikg
Copy link
Member

Brittleness would be a problem indeed. To some extent that also applies to modifications to the vanilla template.
This could be dealt with by using the tools we already use for building. Update .svelte files? That sounds like a job for a markup preprocessor. Modify js/ts code? Rollup does that all day.

Another thing that the typescript modification could add is an extra npm script for validation via tsc --noEmit (and svelte-check?)

Simon Holthausen added 3 commits October 24, 2020 10:02
- add more devDependencies
- add snowpack typescript plugin
- add svelte-preprocess only when TS selected
- add globals.d.ts
- add tsconfig.json
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

I continued work on this setup.

  • add more devDependencies
  • add snowpack typescript plugin
  • add svelte-preprocess only when TS selected
  • add globals.d.ts
  • add tsconfig.json

I did the codemods through simple string-replacements for now. I did not add svelte-check because this could also be added to the snowpack plugin.

"@sveltejs/snowpack-config": "workspace:*",
"@sveltejs/adapter-node": "^0.0.1",
"@sveltejs/kit": "^0.0.1",
"@sveltejs/snowpack-config": "^0.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

I changed these back because they are part of the template, else someone installing from npm would immediately get errors I guess.

Copy link
Member

Choose a reason for hiding this comment

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

^0.0.1 will only match 0.0.1 because of special behavior with leading zeros. We'd want ~0.0.1 for the time being, or 0.0.x, until we advance beyond that point in the versioning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like we can do better still, by adding a prepublishOnly step for this package that grabs the latest versions for all workspace:* dependencies in the template and injects them when a project is scaffolded

Copy link
Member

Choose a reason for hiding this comment

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

Wait, fetches and injects them when you publish the create-svelte package, or fetches and injects them when somebody runs create-svelte?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

I- wh- what's done

Copy link
Member Author

Choose a reason for hiding this comment

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

oh. you may have a point

"esModuleInterop": true,
"skipLibCheck": true,
"forceConsistentCasingInFileNames": true
},
Copy link
Member

@dummdidumm dummdidumm Oct 24, 2020

Choose a reason for hiding this comment

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

I copied the tsconfig from the base repo because I changed the way the *.svelte files are picked up for TS: by adding that definition to the globals.d.ts. That makes the "types": ["svelte"] obsolete. We should change that back someday and sync up with @tsconfig/svelte again. We cannot do this now because it breaks the existing code bases. FYI @orta

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.

7 participants