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

Sveltekit mega change, for better robustness #107

Merged
merged 16 commits into from
Oct 20, 2022

Conversation

Tal500
Copy link
Contributor

@Tal500 Tal500 commented Aug 9, 2022

This is a mega PR for this project, to be more in the "Svelte(Kit) way".

Briefly, what I have done:

  • Move to PNPM (based on move to pnpm for performance #106), use Storybook via Vite (instead of rollup), and fix the storybook-dependency-hell (Storybook uses react, so it's funny that this project have react as a dev dependency now - could be changed to optional dependency, but I'm afraid).
  • Extensive typing on the library, and fix few "safety" issues that I have notices, mainly because of typing.
  • Use SvelteKit packaging mechanism (see my comment)
  • Put the SvelteKit example already in the main dir, via exposing it to "routes" (altought I didn't implement full typing in the example itself). No need to separate the SvelteKit example from the SvelteKit lib. Notice that in pnpm package, only the "lib" directory is being compiled and exported.

Additionally,I have tested importing this library in my own Sapper project, and it works great as it should (meaning that it only depends on Svelte, no need to use SvelteKit for using the library - as a principal).

Please see the updated README.md for the new instructions.
One thing that isn't written there - you may check Svelte&TS typing by the command pnpm check. There is one unresolved issue, that must not be "fixed", otherwise it will break legacy support:

In src\lib\internal\components\Dropzone.svelte:106:
Hint: 'keyCode' is deprecated. (ts)
    // @ts-ignore: can't fix this deprecation, we need this version for legacy support
    if (event.keyCode === 32 || event.keyCode === 13) {
      event.preventDefault();

I'm opening for comments, though it was a hard and a huge work.

fixes #89
fixes #73
maybe also #60
I'm sure it'll solve #45 , didn't test it however.

closes #106

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 9, 2022

I think Netlify fails because it uses old Node.js version. See in https://app.netlify.com/sites/svelte-file-dropzone/deploys/62f24201903fd400087f9dc3#L64-L84:

2:17:42 PM: [svelte-preprocess] Don't forget to install the preprocessors packages that will be used: node-sass/sass, stylus, less, postcss & postcss-load-config, coffeescript, pug, etc...
2:17:42 PM: > @sveltejs/[email protected] postinstall /opt/build/repo/node_modules/@sveltejs/kit
2:17:42 PM: > node svelte-kit.js sync
2:17:42 PM: /opt/build/repo/node_modules/@sveltejs/kit/svelte-kit.js:2
2:17:42 PM: import fs from 'fs';
2:17:42 PM:        ^^
2:17:42 PM: SyntaxError: Unexpected identifier
2:17:42 PM:     at Module._compile (internal/modules/cjs/loader.js:723:23)
2:17:42 PM:     at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
2:17:42 PM:     at Module.load (internal/modules/cjs/loader.js:653:32)
2:17:42 PM:     at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
2:17:42 PM:     at Function.Module._load (internal/modules/cjs/loader.js:585:3)
2:17:42 PM:     at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)
2:17:42 PM:     at startup (internal/bootstrap/node.js:283:19)
2:17:42 PM:     at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)
2:17:43 PM: npm WARN notsup Unsupported engine for [email protected]: wanted: {"node":">= 12"} (current: {"node":"10.24.1","npm":"6.14.12"})
2:17:43 PM: npm WARN notsup Not compatible with your version of node/npm: [email protected]
2:17:43 PM: npm WARN notsup Unsupported engine for @sveltejs/[email protected]: wanted: {"node":">=16.9"} (current: {"node":"10.24.1","npm":"6.14.12"})
2:17:43 PM: npm WARN notsup Not compatible with your version of node/npm: @sveltejs/[email protected]
2:17:43 PM: npm WARN notsup Unsupported engine for [email protected]: wanted: {"node":"^14.18.0 || >=16.0.0"} (current: {"node":"10.24.1","npm":"6.14.12"})
2:17:43 PM: npm WARN notsup Not compatible with your version of node/npm: [email protected]

@thecodejack
Copy link
Owner

Wow..this is awesome. Thanks for creating PR. I will look into this before Sunday.

I think Netlify fails because it uses old Node.js version. See in https://app.netlify.com/sites/svelte-file-dropzone/deploys/62f24201903fd400087f9dc3#L64-L84:

Will see if any upgrade can help.

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 14, 2022

Wow..this is awesome. Thanks for creating PR. I will look into this before Sunday.

I think Netlify fails because it uses old Node.js version. See in https://app.netlify.com/sites/svelte-file-dropzone/deploys/62f24201903fd400087f9dc3#L64-L84:

Will see if any upgrade can help.

Notice that you must run svelte-kit sync after the pnpm installation in Netlify, before the build.

@thecodejack
Copy link
Owner

Put the SvelteKit example already in the main dir, via exposing it to "routes" (altought I didn't implement full typing in the example itself). No need to separate the SvelteKit example from the SvelteKit lib. Notice that in pnpm package, only the "lib" directory is being compiled and exported.

wait. Why do we want to move an example to main parts of repo? This makes our repo look more of App oriented rather library.

@thecodejack
Copy link
Owner

Also don't see much of advantage with vite over rollup except probably a friendly config. Vite internally uses rollup for prod build.

@thecodejack
Copy link
Owner

thecodejack commented Aug 15, 2022

Renaming to ts means REPL support is gone. I would rather prefer typedefs.

Check dasDaniel/svelte-table#97 . They reverted from typescript recently.

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 15, 2022

Renaming to ts means REPL support is gone. I would rather prefer typedefs.

Check dasDaniel/svelte-table#97 . They reverted from typescript recently.

I feel like I need to explain the full story that I've seen trough some Rich Haris's(the Svelte inventor) YouTube video. I can't find this YouTube sadly, so I have to say it briefly:

  1. Rich Haris for some reason loves typing support but prefer JSDoc over typescript.
  2. When Svelte compiles your code, it first preprocess your components (via svelte-preprocess), and then compiles your components to JS. The former step include compiling TS to JS and (optionally) emitting typescript definition file .d.ts.
  3. When you run SvelteKit packaging process, like what I did in this PR, SvelteKit do the following only to the lib directory:
    a. Do the preprocessing step, which also as mentioned compile your component TS script to JS script and emit a .d.ts file..
    b. Compile every .ts file to a .js file.
  4. The idea is that every svelte library developer could use whatever feature he wish when writing the code, but the SvelteKit packaging will "purify" the exported code, like using only JS code but still emit type definition files. This way the user wouldn't need to use typescript in his project(the exported code is in JS), and no matter if he choose to use typescript or not, he'd have typing support and suggestions.
  5. Rich in the mentioned YouTube suggest to put the example/demo/doc code in the other part - in the "routes" directory. This way yo think about programming a normal SvelteKit app, and when you wish to export it as a robust library you use SvelteKit packaging.

All I said is mentioned (with a too short description) in SvelteKit packaging docs.
It's better to see then believe me, that after you run the packaging step you'd see that the "package" dir has only Svelte JS code + typescript defintion files (.d.ts).

The example library you mentioned is just another proof that sadly SvelteKit packaging is purely documented and used. They fixed the problem that everyone have, but still most maintainers don't know the solution exists already.

Another library that actually do the packaging well(I'm also developing this library, but it was like this before I entered) is Svelte Splitpanes.

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 15, 2022

Also don't see much of advantage with vite over rollup except probably a friendly config. Vite internally uses rollup for prod build.

I used Vite for two reasons:

  1. The development mode of Vite (including the one of storybook in Vite) is much faster, since it use ES module in the browser itself, and don't have to bundle them all together every time the source code changes in development mode. BTW: They use ESBuild in the full build step, so even there it's faster than naive rollup.
  2. I figured out that it make no sense to maintain configuration&dependencies both Vite for SvelteKit and Rollup for storybook.

@madeleineostoja
Copy link

When Svelte compiles your code, it first preprocess your components (via svelte-preprocess), and then compiles your components to JS. The former step include compiling TS to JS and (optionally) emitting typescript definition file .d.ts.
When you run SvelteKit packaging process, like what I did in this PR, SvelteKit do the following only to the lib directory:
a. Do the preprocessing step, which also as mentioned compile your component TS script to JS script and emit a .d.ts file..
b. Compile every .ts file to a .js file.
The idea is that every svelte library developer could use whatever feature he wish when writing the code, but the SvelteKit packaging will "purify" the exported code, like using only JS code but still emit type definition files. This way the user wouldn't need to use typescript in his project(the exported code is in JS), and no matter if he choose to use typescript or not, he'd have typing support and suggestions.

Hope I'm not opening a can of worms but just wanted to point out that all of this is true for any compilation step, there's nothing particularly special about sveltekit's packager compared to just running rollup/vite on components, and I find it a slightly odd choice for a small focussed library like this (as opposed to, for eg, a design system)

@thecodejack
Copy link
Owner

thecodejack commented Aug 16, 2022

When Svelte compiles your code, it first preprocess your components (via svelte-preprocess), and then compiles your components to JS. The former step include compiling TS to JS and (optionally) emitting typescript definition file .d.ts.

Ok. If the PR spitting typedefs. I am fine with that. But wanted to makesure it works in REPL, Svelte 3 & Sveltekit without any issues. As @madeleineostoja pointed out, there should be a reason why REPL may have not worked in svelte-table case. Coz they would have had similar setup right.

Rich in the mentioned YouTube suggest to put the example/demo/doc code in the other part - in the "routes" directory. This way yo think about programming a normal SvelteKit app, and when you wish to export it as a robust library you use SvelteKit packaging.

It's not just about Rich recommendation. It makes sense in his suggestion if we don't have storybook. But we have good story book setup which help us easily setup and test. Anyways let me play around this branch and take a call.

Another library that actually do the packaging well(I'm also developing this library, but it was like this before I entered) is Svelte Splitpanes.

Looks like you are the contributor for same there as well 😄 . Nothing wrong just saying. I tried exploring much more popular svelte libs. Didn't find any following same. Anyways, let me explore advantages with the changes you made and get back to you in couple of days.

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 16, 2022

When Svelte compiles your code, it first preprocess your components (via svelte-preprocess), and then compiles your components to JS. The former step include compiling TS to JS and (optionally) emitting typescript definition file .d.ts.
When you run SvelteKit packaging process, like what I did in this PR, SvelteKit do the following only to the lib directory:
a. Do the preprocessing step, which also as mentioned compile your component TS script to JS script and emit a .d.ts file..
b. Compile every .ts file to a .js file.
The idea is that every svelte library developer could use whatever feature he wish when writing the code, but the SvelteKit packaging will "purify" the exported code, like using only JS code but still emit type definition files. This way the user wouldn't need to use typescript in his project(the exported code is in JS), and no matter if he choose to use typescript or not, he'd have typing support and suggestions.

Hope I'm not opening a can of worms but just wanted to point out that all of this is true for any compilation step, there's nothing particularly special about sveltekit's packager compared to just running rollup/vite on components, and I find it a slightly odd choice for a small focussed library like this (as opposed to, for eg, a design system)

The regular Svelte pipeline by rollup/sveltekit/whatever, is to:

  1. Do preprocessing, as I had described
  2. Compile the svelte files to JS file
  3. Bundle all of them together (as part of rollup and Vite ways)

When you run SvelteKit packaging, it do only step 1, and additionally emit typescript def files. The resulted prepared to be published package is at the "package" direcotry.

The idea is that step 1 isn't universally agreeable. Some like typescript, some like scss, some like postcss, some like to use some other high-end custom preprocessing features, and so on.
So one of the benefits using the SvelteKit packaging method, is that you can use whatever features you wish (such as typescript), and then preprocess the svelte files to be in the "pure Svelte component form", only pure JS + CSS + HTML.

When the library user(e.g. REPL) consumes the library, he may still do the preprocessing(as step 1, and then also step 2 and 3) by svelte to your component - but there should not be any noticable effect since they are already in their "pure" form.

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 16, 2022

As @madeleineostoja pointed out, there should be a reason why REPL may have not worked in svelte-table case. Coz they would have had similar setup right.

I think that the reason is simple - this library is "old" from 2019, back when there were no SvelteKit packaging (and even maybe not SvelteKit at all).
In "old" times your method was the best (and the only one), but now SvelteKit made it easier.

It's not just about Rich recommendation. It makes sense in his suggestion if we don't have storybook. But we have good story book setup which help us easily setup and test.

Notice that in this PR I didn't mix the SvelteKit code with storybook, not in directory structure nor in npm script (dev and build for Kit versus storybook and build-storybook for Storybook).
I followed Rich's suggestion to put the SvelteKit example as part of routes (instead of a separated directory), but still didn't touch the dir structure for storybook (that isn't affected at all by SvelteKit).

Looks like you are the contributor for same there as well 😄 . Nothing wrong just saying.

I mentioned before the disclaimer briefly - the maintainer had already followed Rich's suggestion of SvelteKit packaging and the example-in-route-dir since the start of this project, before I had even seen it, so although I'm suspected here, I'm objective though😂

I tried exploring much more popular svelte libs. Didn't find any following same.

There's a new player in town 🤭: https://github.com/chanced/filedrop-svelte

But seriously, see this popular YouTube video and how the new method becomes more and more hot. Since he always try to simplify things, he doesn't explain the advantages using this method.

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 17, 2022

BTW I recommend seeing the newly published library leveluptuts/bookit, though it's still experimental.

It is presented in the following youtube video .

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 17, 2022

Found the video with the misleading name!
Rich Haris's video about packaging (first part of it): https://youtu.be/qD6Pmp45sO4

@Tal500
Copy link
Contributor Author

Tal500 commented Sep 29, 2022

Hi! Any update?

@thecodejack
Copy link
Owner

Hey @Tal500

Sorry. Was occupied with personal stuff. Will be looking into this today.

@thecodejack
Copy link
Owner

BTW I recommend seeing the newly published library leveluptuts/bookit, though it's still experimental.

I like their folder structure. All component files are placed in package folder.

@thecodejack
Copy link
Owner

BTW I recommend seeing the newly published library leveluptuts/bookit, though it's still experimental.

I like their folder structure. All component files are placed in package folder.

I am thinking we should support him by using bookit in our repo as well.

@Tal500 I am moving your code to master-mega and see if we can use bookit on that.

@thecodejack thecodejack changed the base branch from master to master-mega October 20, 2022 08:08
@thecodejack
Copy link
Owner

thecodejack commented Oct 20, 2022

@Tal500 Also note that I might have to remove pnpm usage as I don't use it any repos. It's great but just for one repo can't have another node modules cache folder.

@thecodejack thecodejack merged commit 8c6e783 into thecodejack:master-mega Oct 20, 2022
@Tal500
Copy link
Contributor Author

Tal500 commented Oct 20, 2022

Hi again!

@Tal500 Also note that I might have to remove pnpm usage as I don't use it any repos. It's great but just for one repo can't have another node modules cache folder.

As far as I know, PNPM is 100% compatible with NPM, except:

  • The lockfile is different.
  • The directory structure of node_modules is somehow similar but have compatibility issues with NPM.

That means you need to choose between PNPM/NPM in the repo level, but not in the dependency graph level.

What I mean by the latter, is that a project A can be dependent on the NPM library(notice: Both NPM and PNPM generates 100% NPM library), no matter whether A or B are similar or different by their choices between NPM and PNPM. The reason for that is that theinternal lockfile and the internal node_modules structure isn't being exported to the NPM library.

As an example, sadly due to CI pipeline, I'm using NPM in my website, but some of my dependencies uses PNPM. I couldn't tell this by looking on the final NPM library, but only by looking on their repos.

@thecodejack
Copy link
Owner

My point is about global cache directories. pnpm AFAIK manages it's own like npm and yarn.

@Tal500
Copy link
Contributor Author

Tal500 commented Oct 20, 2022

My point is about global cache directories. pnpm AFAIK manages it's own like npm and yarn.

That's true.
However, is the store of cache really worth it for global usage?

@thecodejack
Copy link
Owner

it's not for global use. cache directories are where you have a copy stored. When you do npm install, cache directory is where npm/pnpm searches first later downloads from internet registries. In pnpm case i think they have specific word called pnpm store directory to which actually links are created.

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.

Typescript support typescript support
3 participants