Rollup TypeScript support #262

mcmxcdev · 2020-09-12T21:21:39Z

Continued pull request of #252.

- fix issue with rollupConfig replace for server config - fix commonjs() part not getting replaced properly - update for tsconfig settings - formatting improvements - file uses tabs instead of spaces now - bump @rollup/plugin-typescript to ^6.0.0 Signed-off-by: mhatvan <[email protected]>

mcmxcdev · 2020-09-12T21:23:36Z

Still todo:

~~add missing types to service-worker.ts~~
~~fix svelte-check warnings and errors~~

benmccann

thanks for working on this! it's been asked for a lot!

benmccann · 2020-09-12T21:25:07Z

scripts/setupTypeScriptRollup.js

+let pkgJSONPath = path.join(projectRoot, "package.json")
+const packageJSON = JSON.parse(fs.readFileSync(pkgJSONPath, "utf8"))
+packageJSON.devDependencies = Object.assign(packageJSON.devDependencies, {
+	"svelte-check": "^1.0.0",


it might be nice to keep these alphabetized with the @ deps coming first

Haha, I actually had it alphabetized but switched it back to conform with the svelte/template one.

benmccann · 2020-09-12T21:25:49Z

scripts/setupTypeScriptRollup.js

+	const elements = fs.readdirSync(dir, { withFileTypes: true })
+
+	for (let i = 0; i < elements.length; i++) {
+		if (elements[i].isDirectory())


can you use curly brackets with all the if / else?

benmccann · 2020-09-12T21:27:58Z

scripts/setupTypeScriptRollup.js

+
+if (fs.existsSync(path.join(projectRoot, "node_modules"))) {
+	console.log(
+		"\nYou will need to re-run your dependency manager to get started."


I think we should just say "npm install" instead of "dependency manager" since that's used in all the Sapper docs in order to help the beginners. Folks that are advanced enough to have chosen yarn or something else will understand how to translate it

Also conforming to: https://github.com/sveltejs/template/blob/master/scripts/setupTypeScript.js

But I can change it if you want!

mcmxcdev · 2020-09-12T21:35:32Z

Currently, svelte-check runs into some issues:

Should I type all of them with the script?

What to do about e.g. post.title? Should I create a models folder in src with a Post.ts that exports an interface that gets used?

This could get quite convoluted once the sapper-template gets bigger.

- add types programmatically to service-worker.ts - add compilerOptions lib for service worker types Signed-off-by: mhatvan <[email protected]>

mcmxcdev · 2020-09-12T23:05:38Z

I pushed another commit, typing out the service-worker.ts file to avoid it from throwing errors that trigger exit code 1

The only thing left now is svelte-check issues, up to the team to decide if it gets done in this pull request or another one.

benmccann · 2020-09-13T02:32:36Z

Hmm. That's an interesting problem. I probably can't answer it alone and would want some of the other maintainers to weigh in.

As a starting point for discussion, my suggestion would probably be that we remove a few of the files (we can probably demonstrate everything we do with fewer files), decide if we're doing sveltejs/sapper#824 since it'd remove one or two of the warnings, remove the unused variables (though others have disagreed stating that they're good for demonstration purposes), and then have the script add types for what's remaining

dummdidumm · 2020-09-13T06:44:57Z

We need to merge the preload typings PR (sveltejs/sapper#1468) so that those typings can be used to type the preload functions. The export let .. things just need proper typings (like number for status I guess)

mcmxcdev · 2020-09-13T06:59:38Z

I will just stand by and wait for instructions.

What to do about occurrences of unknown properties on post?

Could be typed inline or with an external reusable interface file, which would be more best practice.

Steakeye · 2020-09-13T09:40:40Z

In the gist I shared, I demonstrated how I added types to the svelte templates:


// Update service worker ts with `self` declaration
const serviceWorkerPath = path.join(projectRoot, 'src', 'service-worker.ts');
const serviceWorker = fs.readFileSync(serviceWorkerPath, 'utf8');
fs.writeFileSync(serviceWorkerPath, `declare let self: ServiceWorkerGlobalScope\n${serviceWorker}`)

// Switch the *.svelte file to use TS
;[
  {
    view: 'components/Nav',
    vars: [{ name: 'segment', type: 'string' }],
  },
  {
    view: 'routes/_index',
  },
  {
    view: 'routes/about',
  },
  {
    view: 'routes/_layout',
    vars: [{ name: 'segment', type: 'string' }],
  },
  {
    view: 'routes/_error',
    vars: [
      { name: 'status', type: 'number' },
      { name: 'error', type: 'Error' },
    ],
  },
  {
    view: 'routes/blog/_index',
    vars: [{ name: 'posts', type: 'any[]' }],
  },
  {
    view: 'routes/blog/[slug]',
    vars: [{ name: 'post', type: 'any' }],
  },
].forEach(({ view, vars }) => {
  const svelteFilePath = path.join(projectRoot, 'src', `${view}.svelte`);
  let file = fs.readFileSync(svelteFilePath, 'utf8');

  file = file.replace(/(?:<script)(( .*?)*?)>/gm, '<script$1 lang="ts">');

  if (vars) {
    vars.forEach(({ name, type }) => {
      file = file.replace(`export let ${name};`, `export let ${name}: ${type};`);
    });
  }

  fs.writeFileSync(svelteFilePath, file);
});

https://gist.github.com/Steakeye/59db68c59e8fffba788f9de47f12c2ce

mcmxcdev · 2020-09-13T11:22:08Z

Yes thank you, I would use your gist anyways!

but I disagree about typing posts with any[]. It is not showing the intent to enforce best practices to the users about to clone the template

Steakeye · 2020-09-13T14:12:13Z

I agree that I was lazy with my typing of posts, I would probably fix that particular issue as well! And I agree that best practice would be to have a single source of truth across the project, a post.d.ts

- types added in typescript files to please svelte-check script - silence rollup warning for this until type support - remove unused function parameters in preload methods Signed-off-by: mhatvan <[email protected]>

mcmxcdev · 2020-09-21T19:20:30Z

I implemented the changes requested by @benmccann and decided to add types where missing. Now the validate script that runs svelte-check passes without any warnings or errors.

Pull request is ready for another round of reviews.

benmccann · 2020-09-21T19:22:18Z

scripts/setupTypeScriptRollup.js

+	'@types/polka': '^0.5.1',
+	'svelte-check': '^1.0.0',
+	'svelte-preprocess': '^4.0.0',
+	'typescript': '^3.9.3',


should we use TypeScript 4.0 here?

I bumped it to v4, using it without any issues on own project as well.

benmccann · 2020-09-21T19:22:48Z

scripts/setupTypeScriptRollup.js

+
+const projectRoot = argv[2] || path.join(__dirname, '..');
+
+console.log('Adding typescript with rollup');


nit: I'd capitalize TypeScript and Rollup

benmccann · 2020-09-21T19:25:28Z

scripts/setupTypeScriptRollup.js

+// Edit inputs
+rollupConfig = rollupConfig.replace(
+	`onwarn(warning);`,
+	`(warning.code === 'THIS_IS_UNDEFINED') ||\n\tonwarn(warning);`


why do we need this? is it a workaround for sveltejs/sapper#1468 not being merged yet?

Yes, this way we can deliver a completely clean sapper-template with TypeScript and remove the warn ignore once the linked PR is finished.

benmccann · 2020-09-21T19:27:50Z

scripts/setupTypeScriptRollup.js

+
+serviceWorkerConfig = serviceWorkerConfig.replace(
+	`self.skipWaiting();`,
+	`skipWaiting((self as any) as ServiceWorkerGlobalScope);`


I'd probably prefer to do ((self as any) as ServiceWorkerGlobalScope).skipWaiting() vs defining a new method. Same for claimClient

benmccann · 2020-09-21T19:29:38Z

src/routes/blog/index.svelte

-		});
+	export function preload() {
+		return this.fetch(`blog.json`)
+			.then(r => r.json())


I sort of preferred the old indentation here

benmccann

wow, thanks for this! this looks great! I left another set of comments, but I think this will be close to being able to be merged after that

- correct naming for TypeScript and Rollup - bump typescript to v4 and use latest versions for all deps - get rid of custom methods for serviceWorkerConfig Signed-off-by: mhatvan <[email protected]>

mcmxcdev · 2020-09-21T20:43:00Z

Should be solid now ;)

benmccann · 2020-09-21T20:51:25Z

Awesome! Thank you so much for this!

ehrencrona · 2020-09-22T08:09:58Z

scripts/setupTypeScriptRollup.js

+	const svelteFilePath = path.join(projectRoot, 'src', `${view}.svelte`);
+	let file = fs.readFileSync(svelteFilePath, 'utf8');
+
+	file = file.replace(/(?:<script)(( .*?)*?)>/gm, '<script$1 lang="ts">');


If you run the script multiple times (e.g. because it crashes later) it will keep adding multiple lang="ts" attributes. It would be good to check if one already exists.

The script purges itself after one time usage. It is further down in the file: https://github.com/sveltejs/sapper-template/pull/262/files#diff-9b1761910be238be8bde997d8219967bR220

benmccann reviewed Sep 12, 2020

View reviewed changes

- change formatting style to conform to project

2d59e37

- add types programmatically to service-worker.ts - add compilerOptions lib for service worker types Signed-off-by: mhatvan <[email protected]>

benmccann mentioned this pull request Sep 13, 2020

[help-needed] Rollup TypeScript support #252

Closed

- sorted package.json dependencies in setupTypeScriptRollup.js

3ae4591

- types added in typescript files to please svelte-check script - silence rollup warning for this until type support - remove unused function parameters in preload methods Signed-off-by: mhatvan <[email protected]>

mcmxcdev marked this pull request as ready for review September 21, 2020 19:17

benmccann reviewed Sep 21, 2020

View reviewed changes

- change index.svelte back to old indentation

03d7b15

- correct naming for TypeScript and Rollup - bump typescript to v4 and use latest versions for all deps - get rid of custom methods for serviceWorkerConfig Signed-off-by: mhatvan <[email protected]>

Revert spacing changes

acb0080

benmccann merged commit b756d9d into sveltejs:master Sep 21, 2020

ehrencrona reviewed Sep 22, 2020

View reviewed changes


		const projectRoot = argv[2] \|\| path.join(__dirname, '..');

		console.log('Adding typescript with rollup');

Rollup TypeScript support #262

Rollup TypeScript support #262

Uh oh!

Conversation

mcmxcdev commented Sep 12, 2020

Uh oh!

mcmxcdev commented Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benmccann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmxcdev commented Sep 12, 2020

Uh oh!

mcmxcdev commented Sep 12, 2020

Uh oh!

benmccann commented Sep 13, 2020

Uh oh!

dummdidumm commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcmxcdev commented Sep 13, 2020

Uh oh!

Steakeye commented Sep 13, 2020

Uh oh!

mcmxcdev commented Sep 13, 2020

Uh oh!

Steakeye commented Sep 13, 2020

Uh oh!

mcmxcdev commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmxcdev Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann left a comment

Choose a reason for hiding this comment

Uh oh!

mcmxcdev commented Sep 21, 2020

Uh oh!

benmccann commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcmxcdev commented Sep 12, 2020 •

edited

Loading

dummdidumm commented Sep 13, 2020 •

edited

Loading

mcmxcdev Sep 21, 2020 •

edited

Loading

benmccann Sep 21, 2020 •

edited

Loading