svelte-file-dropzone icon indicating copy to clipboard operation
svelte-file-dropzone copied to clipboard

Sveltekit mega change, for better robustness

Open Tal500 opened this issue 1 year ago • 14 comments

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 #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 avatar Aug 09 '22 11:08 Tal500

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]

Tal500 avatar Aug 09 '22 11:08 Tal500

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.

thecodejack avatar Aug 12 '22 08:08 thecodejack

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.

Tal500 avatar Aug 14 '22 17:08 Tal500

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 avatar Aug 15 '22 03:08 thecodejack

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

thecodejack avatar Aug 15 '22 03:08 thecodejack

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

Check https://github.com/dasDaniel/svelte-table/pull/97 . They reverted from typescript recently.

thecodejack avatar Aug 15 '22 03:08 thecodejack

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 avatar Aug 15 '22 07:08 Tal500

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.

Tal500 avatar Aug 15 '22 07:08 Tal500

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)

madeleineostoja avatar Aug 16 '22 03:08 madeleineostoja

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.

thecodejack avatar Aug 16 '22 05:08 thecodejack

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 avatar Aug 16 '22 08:08 Tal500

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 avatar Aug 16 '22 09:08 Tal500

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

It is presented in the following youtube video .

Tal500 avatar Aug 17 '22 19:08 Tal500

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

Tal500 avatar Aug 17 '22 20:08 Tal500

Hi! Any update?

Tal500 avatar Sep 29 '22 11:09 Tal500

Hey @Tal500

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

thecodejack avatar Oct 16 '22 06:10 thecodejack

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 avatar Oct 20 '22 08:10 thecodejack

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 avatar Oct 20 '22 08:10 thecodejack

@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 avatar Oct 20 '22 08:10 thecodejack

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.

Tal500 avatar Oct 20 '22 09:10 Tal500

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

thecodejack avatar Oct 20 '22 16:10 thecodejack

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?

Tal500 avatar Oct 20 '22 16:10 Tal500

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.

thecodejack avatar Oct 20 '22 16:10 thecodejack