Skip to content

Conversation

@ehrencrona
Copy link
Contributor

@ehrencrona ehrencrona commented Oct 27, 2020

  • exposing client code to the project under @sveltejs/kit/client (so e.g. import { goto } from '@sveltejs/kit/client' works)
  • generating typings for the client code using tsc on prepublish
  • made packages/kit build with tsc without errors
  • added types to parts of the code

It only builds typings for packages/kit/src/client since this is presumably the only API we want to expose to project code.

The generated client typing files need to be in the directory client for them to be recognized. I would prefer them to be in e.g. typings, but I don't think there is currently any way to put them elsewhere if we want to generate the typings from source (as suggested by @dummdidumm and with which I agree). I spent quite some time trying to find away around this; this issue convinced me there probably isn't any.

There are two tsconfig.json: one in client that builds the typings for the project code and one in packages/kit that runs tsc over the entire code but doesn't output any build artefact (useful for checking for build warnings/errors).

Building the typings slows down the build by three seconds on my Macbook Air 2019. We could move them out of prepare if we don't want that and possibly put them prepublishOnly instead.

Relates to #31

let data: string;

try {
const result = await snowpack.loadUrl(url, {isSSR: true, encoding: 'utf-8'});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Snowpack's typings show it should be utf8 rather than utf-8

const consumer = new SourceMapConsumer(raw_sourcemap);
// TODO: according to typings, this code cannot work;
// the constructor returns a promise that needs to be awaited
const consumer = new (SourceMapConsumer as any)(raw_sourcemap);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this whole file doesn't seem to be used anyway (copied from sapper). can we just delete it?

Copy link
Member

Choose a reason for hiding this comment

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

perhaps this is because Sapper uses 0.6.1 of source-map. I don't see a version specified in Kit except for source-map: 0.5.7 and source-map: 0.7.3 in pnpm-lock.yaml

before deleting this file we might want to test what happens if an error is thrown server-side in the production app. will the stacktrace be readable (i.e. sourcemapped) or will it be on the minimized code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it's a newer version; then the code most likely does indeed not work. It's non-trivial to change the code to be async, that's why I didn't do it. Should we downgrade in the meantime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the stack traces: in dev mode, if I throw an exception in Nav.svelte, I get this on the server side:

Error: testing stacktrace
    at eval (/_app/components/Nav.js:15:8)
    at Object.$$render (/web_modules/svelte/internal.js:1343:22)

On the client side, it correctly says /Nav.svelte as location.

On a production build, the client-side stack trace from an error I throw in an on:click looks like this:

Uncaught Error: testing stacktraces
    at HTMLButtonElement.<anonymous> (Nav.js:268)
(anonymous) @ Nav.js:268

The server-side stack traces look like:

Error: testing stacktrace
    at fn (/Users/andreasehrencrona/projects/svelte/kit/examples/hn.svelte.dev/.svelte/build/unoptimized/server/_app/components/Nav.js:13:8)

Is this worth a ticket? I'm not sure what expectations we have; neither of the stack traces is terrible, but it would of course be better if it mentioned the correct source file.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we had to stick to 0.6.1 in Sapper because we hadn't figured out how to support newer versions yet

Yeah, it seems worth a ticket to source map the production server-side trace just as Sapper does today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, created #76

'src/setup': '/_app/setup'
},
alias: {
'@sveltejs/kit': '/_app/main',
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean the user has to build once to make TS happy because else the typings don't exist yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this configuration doesn't affect your development tools (this is the run-time module resolution). The (dev) dependency @sveltejs/kit already exists; so the development tools will look inside it for the typings and find them in the client/index.d.ts.

(It won't be able to resolve to the source code since it's only copied into place (in .svelte/build during the build)

@Rich-Harris
Copy link
Member

Thanks — though I'm not certain we do want to expose @sveltejs/kit/client? In #58 I proposed exposing those helpers as e.g. $app/navigation. The reason for that is that those modules depend on generated files, and a special alias like $app communicates that the code you're importing is specific to your app rather than a scoped package like any other.

I'll merge this regardless as it contains a bunch of other useful changes, but we should continue discussing that over on #58.

@Rich-Harris Rich-Harris merged commit e4d6c1e into master Oct 27, 2020
@Rich-Harris Rich-Harris deleted the typings branch October 27, 2020 22:35
@Rich-Harris Rich-Harris mentioned this pull request Oct 27, 2020
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.

5 participants