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

[Doc] Tutorial: Fix typos, snippets and file extensions #8607

Merged
merged 4 commits into from
Feb 17, 2023

Conversation

erikvanderwerf
Copy link
Contributor

@erikvanderwerf erikvanderwerf commented Jan 30, 2023

Fix #8606

  1. Moved the diff marker to the "posts" line, as expected.
  2. Minor spell-checks.
  3. Updated sample TypeScript in Tutorial to compile with typescript@4.9.4

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

docs/Tutorial.md Outdated Show resolved Hide resolved
docs/Tutorial.md Outdated Show resolved Hide resolved
@erikvanderwerf erikvanderwerf requested review from djhi and fzaninotto and removed request for djhi and fzaninotto January 31, 2023 08:33
@erikvanderwerf
Copy link
Contributor Author

^ Sorry! I tried to hit the buttons for both but GitHub is being weird.

docs/Tutorial.md Outdated Show resolved Hide resolved
docs/Tutorial.md Outdated Show resolved Hide resolved
@djhi
Copy link
Collaborator

djhi commented Jan 31, 2023

I'm sorry I should have say so in your initial issue :(

@fzaninotto
Copy link
Member

I'm sorry if I wan't clear. I meant the code should be pure JS, but the files should be .ts and .tsx. When the TS compilation fails with pure JS code, we can add a note after the JS snippet to explain how to modify the file for make it work.

@erikvanderwerf
Copy link
Contributor Author

erikvanderwerf commented Feb 8, 2023

I don't feel comfortable doing that. If the examples are JavaScript then own it. What's the value in the TypeScript extension if you are not actually using it? JavaScript code belongs in .js files, with notes on how to make it work in .ts.

@erikvanderwerf
Copy link
Contributor Author

erikvanderwerf commented Feb 8, 2023

The tutorial directory (https://github.com/marmelab/react-admin/tree/master/examples/tutorial/src) is written with .ts files, so anyone reading both the code and the instructions will notice the difference.

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Also, as asked by @fzaninotto , you should leave all the file extensions as *.ts or *.tsx.

Indeed the tutorial is written so that typescript is enabled, and also the file paths will match the files from the sample repository.

If ever a code snippet, written in pure JS but saved in a TS(X) file raises a warning in your IDE, we simply need to mention it beneath or inside the snippet as comment, just like you did.

docs/Tutorial.md Outdated Show resolved Hide resolved
docs/Tutorial.md Outdated Show resolved Hide resolved
@erikvanderwerf
Copy link
Contributor Author

erikvanderwerf commented Feb 10, 2023

I did not change any file extensions in the repo. If you are referring to the snippets' // in ... comments, it would be misleading to someone making their own project to leave them as TS.

Walk through the tutorial yourself as a new user, like I did. It is confusing to make TypeScript files that do not compile. I would rather understand that the example is written in JS, and any upgrades to TS are on my own. I would understand that more is required from me.

@erikvanderwerf
Copy link
Contributor Author

If the examples are written in TS, why not update the tutorial to reflect that? It would be clearer for everyone.

@slax57
Copy link
Contributor

slax57 commented Feb 13, 2023

@erikvanderwerf Yes I am referring to the snippets' // in ... comments

I know it's a bit confusing, because we are currently between two seats. But I'll try my best to explain.

  • We cannot (for now) change the snippets to TS, for technical reasons (they are not yet supported by our code highlighter) and also because we do not want to turn off people who are still not ready to use TS and only know JS
  • We name our files *.ts and *.tsx because, on the other hand, we want to encourage the reader to use TS (since it's a good opportunity because they are starting a new project) and also because they match the file names from the tutorial example in the repo
  • So we end up in this compromise of a solution that consists in having JS code inside .ts files. But as you saw, only a handful of files require adjustments to get rid of the TS warnings. Most of the snippets can be copy-pasted with no issues at all. That's why we consider it to be a good compromise for now.

Hopefully this will make it clearer for you why we ask for these changes.

@erikvanderwerf
Copy link
Contributor Author

erikvanderwerf commented Feb 16, 2023

Technological challenges are a valid reason for not upgrading the Tutorial code blocks 😄

So if I understand correctly:

  1. The code block needs to indicate JS for syntax highlighting purposes (```js).
  2. But the comment // in *.ts will indicate TS to encourage people to use TS.
  3. But the code as written will be JS, meaning that TS might not compile if a new user copies the code.
  4. But with comments in the JS explaining the changes needed so that it would compile in TS.

It certainly is confusing (and I don't exactly agree, but I can live with it). If the above understanding is correct, I'll make the appropriate changes.

@slax57
Copy link
Contributor

slax57 commented Feb 16, 2023

@erikvanderwerf Yes that's correct!
Thanks 🙂

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thanks!

@slax57 slax57 changed the title #8606 Touching up Tutorial.md with improvements to TypeScript examples. [Doc] Tutorial: Fix typos, snippets and file extensions Feb 17, 2023
@slax57 slax57 added this to the 4.8.1 milestone Feb 17, 2023
@slax57 slax57 merged commit 7b74c64 into marmelab:master Feb 17, 2023
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.

Tutorial Posts and authProvider
4 participants