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

update svelte skeleton #12350

Merged
merged 9 commits into from Dec 13, 2022
Merged

update svelte skeleton #12350

merged 9 commits into from Dec 13, 2022

Conversation

tosinek
Copy link
Contributor

@tosinek tosinek commented Dec 10, 2022

The Svelte integration seems to have issues when both zodern:melte and rdb:svelte-meteor-data are used together. It is quite random, some stuff will just stop being reactive, while some other still works. I think it was even breaking HMR in a Blaze+Svelte project for me. When using just one package, all the problems suddenly disappear.

I suggest updating the skeleton to only include zodern:melte, which is also aligned with the guide at https://svelte-tutorial.meteor.com/

Related issues:
zodern/melte#24
zodern/melte#25

I also updated the skeleton to utilize the collection. Not really necessary, but if it is not used, then it might be removed completely instead?

@CLAassistant
Copy link

CLAassistant commented Dec 10, 2022

CLA assistant check
All committers have signed the CLA.

@tosinek
Copy link
Contributor Author

tosinek commented Dec 11, 2022

I can see this table would need updating too
https://docs.meteor.com/commandline.html#meteorcreate

@Grubba27
Copy link
Contributor

Loved this change! Update the docs(that table).
In the next meteor patch release I will make sure that this comes up too

Copy link
Contributor

@Grubba27 Grubba27 left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 9 to 15
const subIsReady = true; // remove this line if you want to use the code below
// no need to publish/subscribe as there is autopublish package installed
// let subIsReady = false;
// $m: {
// const handle = Meteor.subscribe('links.all'); // todo: setup the server-side publication
// subIsReady = handle.ready();
// }
Copy link
Member

Choose a reason for hiding this comment

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

By default, skeletons no longer have autopublish. It might be good to have this uncommented.

Personally I like to separate those two lines into their own statements like this, though it doesn't really matter.

$m: handle = Meteor.subscribe('links.all');
$m: ready = handle.ready();

Copy link
Member

Choose a reason for hiding this comment

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

It might also be good to add the server code for the publication.

// subIsReady = handle.ready();
// }

// $m is available from zodern:melte package
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to have a short explanation of what $m is, or a link to the docs for it.

@zodern
Copy link
Member

zodern commented Dec 12, 2022

I've been wondering if we should configure the svelte skeleton so it supports typescript. It doesn't need to have any typescript code, but at least have the tsconfig and dependencies installed so you don't have to do anything to start using typescript.

@tosinek
Copy link
Contributor Author

tosinek commented Dec 12, 2022

I've been wondering if we should configure the svelte skeleton so it supports typescript. It doesn't need to have any typescript code, but at least have the tsconfig and dependencies installed so you don't have to do anything to start using typescript.

Great idea. I am thinking about a whole guide for Svelte, like the one for React https://guide.meteor.com/react.html to document TS, SSR, i18n, reactivity, blaze integration or routing

@Grubba27 Grubba27 changed the base branch from devel to release-2.9.1 December 13, 2022 14:17
@Grubba27
Copy link
Contributor

Is there something missing? @tosinek ? can I merge this to 2.9.1?

@tosinek
Copy link
Contributor Author

tosinek commented Dec 13, 2022

I also wanted to add Typescript. Thinking whether it is better to just prepare it for TS or convert the codebase to TS.
Or would it be better just to document it somewhere else?

@Grubba27
Copy link
Contributor

Grubba27 commented Dec 13, 2022

IMO @tosinek, I would convert it to TypeScript, the same way we have with React-TS.
By default,I think we should enforce type-safety

@tosinek
Copy link
Contributor Author

tosinek commented Dec 13, 2022

Ok, few more minutes :)

@Grubba27
Copy link
Contributor

Ci wont work but I will take a look in the 2.9.1 branch. Just let me know when you are done :)

@tosinek
Copy link
Contributor Author

tosinek commented Dec 13, 2022

I think I am done. The server/main.js could be still converted, but may leave it to another update

@Grubba27
Copy link
Contributor

You can copy from the other skeleton

@tosinek
Copy link
Contributor Author

tosinek commented Dec 13, 2022

I think it is time to merge it as it is. Importing .svelte files in .ts files is making TS to complain, so I will have to figure it out, but not tonight.

@Grubba27
Copy link
Contributor

Got it! Thx for your work @tosinek! hope to see you again :)

@Grubba27 Grubba27 merged commit 76d9080 into meteor:release-2.9.1 Dec 13, 2022
@Grubba27 Grubba27 mentioned this pull request Dec 13, 2022
@tosinek
Copy link
Contributor Author

tosinek commented Dec 13, 2022

I wanna create a Svelte guide. Would it make sense to have routing in this skeleton? It is something I was missing a lot in the beginning. I can see that the new Vue skeleton includes routing. Maybe there could even be some kind of guideline on what to include in every skeleton? (eg. that the we are trying to enforce type safety)

@Grubba27
Copy link
Contributor

We want to maintain a standard for every skeleton, so I think this is a nice one. Most apps will need a router! I make issues so the community can help with other frameworks.

Also, while talking with the team, we decided that for now, it could be nice to have the tsconfig.json already done so that to make your app a TypeScript, one would just need to add the ts files. I will make the change, for now, to revert the links.ts to links.js.

@tosinek
Copy link
Contributor Author

tosinek commented Dec 13, 2022

I think .ts works fine for this skeleton, except for the main client entrypoint that is still .js. It does not like the App.svelte import there. Links.ts work fine on my machine.

And I created a minimalistic tsconfig.json with comments that seems to be working (included in the PR). Because it is difficult for me to use a long config with many options I don't even understand (eg the one from the Meteor guide) without knowing what is important there and what is just a preference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants