Closed lubiah closed 1 year ago
hi @kudadam, thank you very much for your contribution, I appreciate it very much!
I will review your changes this weekend.
@artiebits do look and tell me if there are any more changes to make
@artiebits , am still waiting on your review about the pull request
@artiebits , do you want me to remove the default prettier configuration file??
@artiebits, the default prettier settings has been ran on the whole code base. Am still working on the readme though
When I do nom run check
it throws errors:
Error: Cannot find name 'SvelteSeo'.
/**@type {SvelteSeo['twitter']} */
twitter: {
/Users/arturkhusaenov/code/personal/svelte-seo/src/routes/index.js:53:13
Error: Cannot find name 'SvelteSeo'.
/**@type {SvelteSeo['openGraph']} */
openGraph: {
====================================
svelte-check found 2 errors and 0 warnings
when I run npm run test
it throws:
[WebServer] /bin/sh: pnpm: command not found
Error: Process from config.webServer was not able to start. Exit code: 127
82 skipped
1 error was not a part of any test, see above for details
Do you have pnpm installed globally?
Do you think it would be possible to create pages for article, video, profile, etc, like here https://github.com/garmeeh/next-seo/tree/master/e2e/pages? It might be better then adding all tags into one single file.
The rest looks fine to me. if we fix these final things than I would love to merge your PR.
And sorry for keeping you waiting like this!
Alright, I will do that tonight
Do you have pnpm installed globally?
will current travisCI work with pnpm or it needs to be updated?
It needs to be updated
Is travis used to run the tests??
why some files are js, and some are ts?
Do you think it would be possible to create pages for article, video, profile, etc, like here https://github.com/garmeeh/next-seo/tree/master/e2e/pages? It might be better then adding all tags into one single file.
The rest looks fine to me. if we fix these final things than I would love to merge your PR.
And sorry for keeping you waiting like this!
Upon careful consideration, it wouldn't be a good idea. I know you were inspired by that repo when creating this but trying to emulate their everything wouldn't help. The less the code, the fewer the bugs, this method already works so why should we split them?? Also, if you are to split them all into equal files, it would mean more test files to write to accomplish what is already accomplished now. The reason there is only one file is to unify everything for a single source of truth. I do not know if this is convincing you but it's the best. Let's try to reduce the code we write so that maintaining the code will be much easier.
If you create a page for each open graph tag, then you would also have to create pages for dynamic twitter tags, then you will also create pages for different JSON-LD's, it's just going to bloat the codebase.
Another issue is that, you are barely around for this repo. I opened this issue months ago yet it does not seem to get anywhere closer to completion. The main reason why I opened this pull request was to add improvements to your module which it was lacking. I had a project which needed these changes but because of your delay. I had to look for an alternative. If you wouldn't always be around for your code, you could at least give maintainers access to the repo so important bugs and fixes can be made.
These are the trends between this plugin and another alternative. It's getting more downloads. Development on this repo is also uncertain, I don't know if you are getting me but trying to make everything smooth for anyone who wants to contribute to your code.
Do you think it would be possible to create pages for article, video, profile, etc, like here https://github.com/garmeeh/next-seo/tree/master/e2e/pages? It might be better then adding all tags into one single file. The rest looks fine to me. if we fix these final things than I would love to merge your PR. And sorry for keeping you waiting like this!
Upon careful consideration, it wouldn't be a good idea. I know you were inspired by that repo when creating this but trying to emulate their everything wouldn't help. The less the code, the fewer the bugs, this method already works so why should we split them?? Also, if you are to split them all into equal files, it would mean more test files to write to accomplish what is already accomplished now. The reason there is only one file is to unify everything for a single source of truth. I do not know if this is convincing you but it's the best. Let's try to reduce the code we write so that maintaining the code will be much easier.
If you create a page for each open graph tag, then you would also have to create pages for dynamic twitter tags, then you will also create pages for different JSON-LD's, it's just going to bloat the codebase.
Another issue is that, you are barely around for this repo. I opened this issue months ago yet it does not seem to get anywhere closer to completion. The main reason why I opened this pull request was to add improvements to your module which it was lacking. I had a project which needed these changes but because of your delay. I had to look for an alternative. If you wouldn't always be around for your code, you could at least give maintainers access to the repo so important bugs and fixes can be made.
These are the trends between this plugin and another alternative. It's getting more downloads. Development on this repo is also uncertain, I don't know if you are getting me but trying to make everything smooth for anyone who wants to contribute to your code.
hi Lucretius,
Good point, let's not make this PR bigger, since it's already big.
Thanks for your contribution, and I understand your frustration about time it takes to review your PR, but please understand me too. Before you started working on it I asked to keep changes as small as possible so it would be easier for me to review them. However, instead of rewriting this package step by step you open one big PR, and changed the formatting which touched every single file.
Let's try to fix merge conflicts, CICD, errors in console, and I think we are good to go! Again, thank you very much for your contribution, not that much left, let's finish it and merge to mater
Do you think it would be possible to create pages for article, video, profile, etc, like here https://github.com/garmeeh/next-seo/tree/master/e2e/pages? It might be better then adding all tags into one single file. The rest looks fine to me. if we fix these final things than I would love to merge your PR. And sorry for keeping you waiting like this!
Upon careful consideration, it wouldn't be a good idea. I know you were inspired by that repo when creating this but trying to emulate their everything wouldn't help. The less the code, the fewer the bugs, this method already works so why should we split them?? Also, if you are to split them all into equal files, it would mean more test files to write to accomplish what is already accomplished now. The reason there is only one file is to unify everything for a single source of truth. I do not know if this is convincing you but it's the best. Let's try to reduce the code we write so that maintaining the code will be much easier. If you create a page for each open graph tag, then you would also have to create pages for dynamic twitter tags, then you will also create pages for different JSON-LD's, it's just going to bloat the codebase. Another issue is that, you are barely around for this repo. I opened this issue months ago yet it does not seem to get anywhere closer to completion. The main reason why I opened this pull request was to add improvements to your module which it was lacking. I had a project which needed these changes but because of your delay. I had to look for an alternative. If you wouldn't always be around for your code, you could at least give maintainers access to the repo so important bugs and fixes can be made.
These are the trends between this plugin and another alternative. It's getting more downloads. Development on this repo is also uncertain, I don't know if you are getting me but trying to make everything smooth for anyone who wants to contribute to your code.
hi Lucretius,
Good point, let's not make this PR bigger, since it's already big.
Thanks for your contribution, and I understand your frustration about time it takes to review your PR, but please understand me too. Before you started working on it I asked to keep changes as small as possible so it would be easier for me to review them. However, instead of rewriting this package step by step you open one big PR, and changed the formatting which touched every single file.
Let's try to fix merge conflicts, CICD, errors in console, and I think we are good to go! Again, thank you very much for your contribution, not that much left, let's finish it and merge to mater
Hi, you are right. I shouldn't have opened the pull request with much code.
The testing, why don't you use github actions?? Wouldn't that be simpler??
Hi @kudadam these are the only things that left. let me know if I can help you with some of these things
yarn run check
and npm run check
throws errors:
Error: Cannot find name 'SvelteSeo'.
/**@type {SvelteSeo['twitter']} */
twitter: {
/Users/arturkhusaenov/code/personal/svelte-seo/src/routes/index.js:53:13 Error: Cannot find name 'SvelteSeo'.
/*@type {SvelteSeo['openGraph']} / openGraph: {
==================================== svelte-check found 2 errors and 0 warnings
2. `yarn test` and `npm run test` throws:
[WebServer] /bin/sh: pnpm: command not found Error: Process from config.webServer was not able to start. Exit code: 127
82 skipped 1 error was not a part of any test, see above for details
3. update travis.yml to use new commands
add these
remove
override:
yarn
againDo you have pnpm installed globally?
I do, pnpm throws those errors too
Hi @kudadam these are the only things that left. let me know if I can help you with some of these things
yarn run check
andnpm run check
throws errors:Error: Cannot find name 'SvelteSeo'. /**@type {SvelteSeo['twitter']} */ twitter: { /Users/arturkhusaenov/code/personal/svelte-seo/src/routes/index.js:53:13 Error: Cannot find name 'SvelteSeo'. /**@type {SvelteSeo['openGraph']} */ openGraph: { ==================================== svelte-check found 2 errors and 0 warnings
yarn test
andnpm run test
throws:[WebServer] /bin/sh: pnpm: command not found Error: Process from config.webServer was not able to start. Exit code: 127 82 skipped 1 error was not a part of any test, see above for details
- update travis.yml to use new commands add these
- build - check - test - lint
remove
# install dependencies and check that Cypress can run override: - npm ci - npm run cy:verify
- resolve the conflict in yarn.lock. I guess you can remove it and do
yarn
again
The reason why number 2 is not working is because, it's set to use pnpm. It should be pnpm test
.
Hey, quick question, which package manager should we use, pnpm, npm or yarn.
Talking of travis, why don't you use Github actions for the tests?? I can see you use it for publishing the package so it would be more nice to just unify the whole thing or what do you think
The reason why number 2 is not working is because, it's set to use pnpm. It should be
pnpm test
.Hey, quick question, which package manager should we use, pnpm, npm or yarn.
Talking of travis, why don't you use Github actions for the tests??
pnpm run test
and pnpm run
throw the same errors.great, thanks. please resolve the conflict in yarn.lock so I will be able to merge your PR. I guess you can remove yarn.lock and do yarn
again
I just tried, it didn't work
I have fixed the conflict
After all the tests passes, don't merge yet. Go through the readme to see if you would make any changes
I don't know how to debug Travis CI so you can at least tell me why the tests are failing
I don't know how to debug Travis CI so you can at least tell me why the tests are failing
it's actually very simple, click on Details of the failed job and t will open a page with a few links. For example "View more details on Travis CI".
Or use this link https://app.travis-ci.com/github/artiebits/svelte-seo.
The error happens because of incompatible node versions error @sveltejs/kit@1.2.6: The engine "node" is incompatible with this module. Expected version "^16.14 || >=18". Got "17.9.1"
One small remark, you don't need npm run format
in Travis config file, because there is no point in formatting files in CI.
please try to downgrade to 16.14
The command "npm config set progress false" failed and exited with 1 during . What does that mean
I think I know what is happening, the VM travis is trying to use can't install node 17. Yarn also requires 18
npm run lint didn't work. You need to run format before
npm run lint didn't work. You need to run format before
it didn't work because not all files follow formatting rules. you need to run npm run format
locally and push the changes.
having npm run format
in CI doesn't make sense because then you can push your code without following formatting rules but it will always pass in CI.
Hurrayπ₯πππππππ
Remeber, go through readme before you push to master
I've published a beta version with your changes svelte-seo@2.0.0-beta.0
let's test it and merge your contribution to master!
I've published a beta version with your changes
svelte-seo@2.0.0-beta.0
let's test it and merge your contribution to master!
alright
However, there are some breaking changes I think we need to specify
hi @kudadam I am testing svelte-seo@2.0.0-beta.0
and getting Failed to resolve entry for package "svelte-seo". The package may have incorrect main/module/exports specified in its package.json.
Does it work for you?
hi @kudadam I am testing
svelte-seo@2.0.0-beta.0
and gettingFailed to resolve entry for package "svelte-seo". The package may have incorrect main/module/exports specified in its package.json.
Does it work for you?
Yes, I'm getting the same error.
I think it's how you built the package.
When you run yarn run build
. SvelteKit packages the package into a folder name package
inside the root directory. That is what you need to publish
When you run
yarn run build
. SvelteKit packages the package into a folder namepackage
inside the root directory. That is what you need to publish
you are right. there was a configuration in package.json but you deleted it:
"module": "dist/index.min.mjs",
"main": "dist/index.min.js",
"types": "types/index.d.ts",
Please add the relevant configuration to the package.json and test it by building and importing the package to some test project locally.
I will be on vacation until February 8th! see you later
When you run
yarn run build
. SvelteKit packages the package into a folder namepackage
inside the root directory. That is what you need to publishyou are right. there was a configuration in package.json but you deleted it:
"module": "dist/index.min.mjs", "main": "dist/index.min.js", "types": "types/index.d.ts",
Please add the relevant configuration to the package.json and test it by building and importing the package to some test project locally.
I will be on vacation until February 8th! see you later
No @artiebits, you got it all wrong, I am not supposed to add these scripts to the package.json. When you run yarn build
. It runs a command called svelte-package
which is different from the command which builds svelte apps.
It generates a folder called package. Inside the generated folder, it contains another package.json
with the scripts above.
The problem was how you deployed the package.
Even the
svelte-package
command says we should change into the package directory and publish it instead of the root repository.
This is how the package directory looks like.
This is also how the package.json inside the package directory looks like. I am not supposed to add those scripts for it to work, besides, SvelteKit doesn't generate a folder like dist
or something.
I have tried publishing the package folder and it worked. Try to see if you can do same and the issue would be resolved.
Happy vacation!
you got it all wrong, I am not supposed to add these scripts to the package.json
Okay :) thank you for explaining it. I have published a new version, it looks good to me. I would say I am ready to merge it to master and release a new version!
Could you please make a list of changes/new features that you have made? It would help me to create a changelog. Thanks!
Hey, @artiebits, I'm glad everything went well and we're ready to publish. Before doing so, we also need to specify the breaking changes. I will list all the changes I made here.
Changes
base
(Allows you to set the base URL)applicationName
(Allows you to set the web app's name in the head)themeColor
(Allows you to set the theme colour of the website)nositelinkssearchbox
(Prevents google from showing a search box for your website.notranslate
(Prevents google from translating your website)amp
(Allows you to set the URL to the amp version of the page.manifest
(Set URL to your website's manifest.json)languageAlternates
(Set alternative language versions of the pagefacebook
(Set's your facebook app ID)openGraph.article.publishedTime
to openGraph.article.published_time
openGraph.article.modifiedTime
to openGraph.article.modified_time
.openGraph.article.expirationTime
to openGraph.article.expiration_time
.openGraph.article.authors
to openGraph.article.author
(this change was very important, the one letter changes saves approx 7 lines of code.openGraph.article.tags
to openGraph.article.tag
.So far these are all I've found, I will add more as I find some along the way.
Also, before we publish, I think we need to state the breaking changes in the README.md
thank you for your contribution! https://github.com/artiebits/svelte-seo/releases/tag/v1.5.0
@artiebits , Finally, I am done with the proposed new version I suggested in #44.
It comes with new features plus some breaking changes, not many, I think one or two properties' names were renamed. Here are the new features.
The coding style in this new version was changed a bit. For example, in the old version, we used to use if statements to check if a property existed, before adding its meta tag but in this version, we loop through all the properties provided by the user. You might ask what if the user provides a property which isn't specified? Well, since the component is well-typed. The user will be notified of the error.
Also, for the breaking changes, I had to change some of the variable names. For example,
openGraph.article.publishedTime
was changed toopenGraph.article.published_time
. The reason is simple, we now use a custom parser to convert camel case to the required case so every capital letter is placed by ":" + the letter since we are now using loops.If we use
openGraph.article.publishedTime
, the parser will convert it toarticle:published:time
, since this is not valid, I need to change some of the variable names.Also, I started the project from a fresh sveltekit repository so I advice we work on this and fix any changes before we merge it as a new version.
Talking about version, since this change is going to include breaking changes, I suggest we name this version 2.0.0 before any other fixes are added.
Also, this project uses playwright tests to tests every single property.
If you have any more questions, feel free to ask me