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

Remove function wrapper / add matching dprint and eslint configurations #7596

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

pmario
Copy link
Member

@pmario pmario commented Jul 9, 2023

This PR was created to be able to test "dprint" with the TW repo. That's why dprint formatting is not part of the PR.

This PR is related to: Use DPrint to ensure consistent code formatting #7474 ... But there are some differences

  • Only 1 external dependency is added to the developer configuration
    • dprint
  • improved dprint.json configuration
  • improved eslintrc.json configuration
    • changed from eslintrc.yml to eslintrc.json, which is much more readable
  • It's possible and suggested to use eslint with VScode for syntax linting, but eslint settings do not interfere with dprint.
  • with PR Use DPrint to ensure consistent code formatting #7474 dprint and eslint interfere heavily. So it's impossible to work with it in VSCode
  • Using dprint only is also approximately 60 times faster.

This PR removes the function wrapper from all .js modules, where possible.

(function(){
...
})();
  • All TW tests pass
  • All commonjs tests pass
  • TW Node.js version works as expected.
  • tiddlywiki edtions/tw.com --build does create working wikis.

After downloading this branch it's possible to use the "dprint fmt" command from the TiddlyWiki5 directory.
So after switching to this branch you can do the following:

npm install
dprint fmt

It will download the the dprint typescript, JSON and markdown wasm plugins.
Then it will "dprint" the whole TW project.

It will modify 525 files and pretty-print them. I did manually compare and review them 1 by 1. It looks good and it does improve code readability

All in all the configurations are very close to the existing TW styleguide. There is 1 major difference. dprint adds spaces between function arguments. There is no setting at the moment, that would allow us to change that.

eg:

var Command = function(params,commander) {

is converted to see the space between params and commander

var Command = function(params, commander) {

@linonetwo did create an issue at the dprint repo: dprint/dprint-plugin-typescript#526 ... There was no response yet. But it may be configurable in the future.

  • All TW test pass
  • Node.js version seems to work

There is an dprint error message atm, which can be ignored for this PR. But we should create an issue at the dprint repo in the future.

Internal dprint error, that should be reported to dprint repo
PS E:\git\tiddly\tiddlywiki\TiddlyWiki5> dprint fmt
Error formatting E:\git\tiddly\tiddlywiki\TiddlyWiki5\plugins\tiddlywiki\twitter-archivist\archivist.js. Message: Formatting succeeded initially, but failed when ensuring a stable format. This is most likely a bug in the plugin where the text it produces is not syntatically correct. Please report this as a bug to the plugin that formatted this file.

Expression expected at E:/git/tiddly/tiddlywiki/TiddlyWiki5/plugins/tiddlywiki/twitter-archivist/archivist.js:233:13

        forawait (const [filename, fileHandle] of dirHandle.entries()) {
             ~~~~~
Had 1 error(s) formatting.

@vercel
Copy link

vercel bot commented Jul 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ❌ Failed (Inspect) Jun 14, 2024 11:16am

@pmario
Copy link
Member Author

pmario commented Jul 9, 2023

@linonetwo ... Can you have a closer look? What do you think?

@pmario
Copy link
Member Author

pmario commented Jul 9, 2023

From the size comparisons we can see, that removing the wrappers and adding dprinted formatting we end up with the same size as we have now.

wiki file size dprinted wrapper removed w-r dprinted
v5.3.1-pre index.html 8.028 8.125 8.019 8.026
v5.3.1-pre empty.html 2.404 2.497 2.395 2.401
v5.3.1-pre tiddlywikicore-5.3.1-pre.js 2.264 2.358 2.255 2.261

@pmario
Copy link
Member Author

pmario commented Jul 9, 2023

dprint configuration settings can be found at: https://dprint.dev/plugins/typescript/config/
eslint config at: https://eslint.org/docs/latest/use/configure/

@linonetwo
Copy link
Contributor

linonetwo commented Jul 10, 2023

I think you should use dprint integration with eslint, otherwise eslint formatting may conflict with eslint's. And eslint provides a unified autofix alt+shift+G (may need to config hotkey), which autofix eslint problem with dprint formatting problem in VSCode.

If you separate them, each code will require 2 hot key to format, which will make it annoying to code in tw.

  • Using dprint only is also approximately 60 times faster.

This is because eslint also autofix many other problems, not only formatting. But this may not be desired. But in the future, I think we should use dprint binding with eslint for dev experience in VSCode.

@linonetwo
Copy link
Contributor

It will modify 525 files and pretty-print them. I did manually compare and review them 1 by 1. It looks good and it does improve code readability

This is very helpful.

@pmario
Copy link
Member Author

pmario commented Jul 10, 2023

I think you should use dprint integration with eslint, otherwise eslint formatting may conflict with eslint's.

I did use the combination with dprint plugins for eslint. The disadvantage is that it needed 2 minutes to do the formatting that dprint alone can do in 2 seconds with absolutely the same result.

I did have a big problem with VSCode, because dprint setting and eslint setting did work against each other. With the settings I did add to dprint.json eslint is not needed for prettyprinting.

ESLint can be installed as a plugin to VSCode. Dprint can be installed to VSCode and they work together without any fight. But the CLI needs to be dprint alone. It's much faster and does not produce any conflicts.

@linonetwo
Copy link
Contributor

The disadvantage is that it needed 2 minutes to do the formatting that dprint alone can do in 2 seconds with absolutely the same result.

This is only the first time. You can do this just using dprint, but we need to add back integration later. And it only takes 0.xS in daily use.

dprint setting and eslint setting did work against each other. With the settings I did add to dprint.json eslint is not needed for prettyprinting.

You can copy the config from my PR, which don't have any conflict, acturally I already made a https://github.com/tiddly-gittly/eslint-config-tidgi and https://github.com/tiddly-gittly/TidGi-Desktop/blob/406062b83262440e9ad8708205d6d03af5ff7ec9/.eslintrc.js#L36-L40 without any issue.

But the CLI needs to be dprint alone. It's much faster and does not produce any conflicts.

Yes you can do this for the first format in this PR.

@pmario
Copy link
Member Author

pmario commented Jul 10, 2023

As I wrote with your configuration there where conflicts.

@pmario
Copy link
Member Author

pmario commented Jul 10, 2023

You can do this just using dprint, but we need to add back integration later.

There is not need to add eslint as a dev dependency to TW package.json. It's OK to install the ESLint and the DPrint extensions to VSCode, to get all the info in VSCode and when the code is saved it is prettyprinted and ESLint does it work too.

But when TW is created from the command line, dprint is the only dependency we need as this PR shows.

@linonetwo
Copy link
Contributor

linonetwo commented Jul 10, 2023

There is not need to add eslint as a dev dependency to TW package.json

As long as you don't need those autofix. I use tons of eslint plugin because my time is precious, so I automate as many things as I can. I think tw devs are too, so I tend to introduce those dev tools.

But you work more in tw codebase as I did, so it is up to you. I mostly work in TidGi and https://github.com/tiddly-gittly/Modern.TiddlyDev plugin environments now, and I have full set of https://github.com/tiddly-gittly/eslint-config-tidgi on them, so I don't have strong opinion here in tw core codebase.

your configuration there where conflicts.

what is that? We can fix it instead of just drop it

@pmario
Copy link
Member Author

pmario commented Jul 10, 2023

As long as you don't need those autofix.

ESLint autofix are done as soon as you save a code file, if the ESLint extension is installed in VSCode and the configuration setting is activated.

... with your configuration there where conflicts.

what is that? We can fix it instead of just drop it

It seems that the dprint-eslint-plugin wanted to add 1 extra line at the end of every code file but eslint had a problem with that. So in the end every save the number of lines at the end changed. I could not fix that with any configuration setting.

For some constructions like spaces between commands and braces eg: switch () ... there is no setting in dprint atm, to avoid it. So settings like these will always conflict between dprint and eslint. So with every save the space will be toggled. With 1 save dprint wins with the next save eslint wins ... That's super annoying and not necessary.

I think we do not need eslint-dprint-integration on the CLI level.

IMO dprint is enough to get a consistent code style, with the dprint.json setting in this PR.

@linonetwo
Copy link
Contributor

linonetwo commented Jul 10, 2023

It seems that the dprint-eslint-plugin wanted to add 1 extra line at the end of every code file but eslint had a problem with that. So in the end every save the number of lines at the end changed. I could not fix that with any configuration setting.

So settings like these will always conflict between dprint and eslint. So with every save the space will be toggled. With 1 save dprint wins with the next save eslint wins ... That's super annoying and not necessary.

Is dprint-integration properly installed? plugin:dprint-integration/disable-conflict should disable all eslint rules related to formatting. see so1ve/eslint-plugin-dprint-integration#11 (comment)

I don't know this happened last time, I will take time to check this. But I think you might not have packages properly installed.

I think we do not need eslint-dprint-integration on the CLI level.

This is OK, but we should use it with VSCode. CLI level only need to be done this time in this PR
, and in the future it works in VSCode and CI.

@pmario
Copy link
Member Author

pmario commented Jul 10, 2023

Is dprint-integration properly installed? plugin:dprint-integration/disable-conflict should disable all eslint rules related to formatting. see so1ve/eslint-plugin-dprint-integration#11 (comment)

Yes it was installed and configured properly. But if eslint rules are disabled anyway, then we do not need them. So we do not need the extension. It does not make sense.

eslint-plugin-dprint-integration seems to be no official ESLint plugin.

I do not want to invest time into debugging 3rd party extensions and mess with incompatible settings. It's not worth it. Removing the dependency imo will be the best choice in the long run.

It will be enough work to keep dprint up to date.

@Jermolene
Copy link
Member

Hi @pmario I think it might be easier to review this PR if we could remove the whitespace updates, and just include the changes necessary to get dprint installed and running.

@pmario
Copy link
Member Author

pmario commented Oct 31, 2023

Hi @pmario I think it might be easier to review this PR if we could remove the whitespace updates, and just include the changes necessary to get dprint installed and running.

As soon as dprint is active it will destroy the formatting for every tiddler because of the function wrapper

(function(){
...
})();

So IMO we cannot test the one without the other. -- I'll add a list of tidders here in the comment section, that contains the changes. Then the rest will be function-wrapper fixes.

@Jermolene
Copy link
Member

Thanks @pmario it would make the commit history very confusing. Why can't we merge the dprint integration in one PR and then immediately after merge the changes that it makes on first run?

Thinking about it, I thought we had decided to remove the function wrappers from modules in any case? In which case that should be done first as a separate PR?

@pmario
Copy link
Member Author

pmario commented Oct 31, 2023

Thinking about it, I thought we had decided to remove the function wrappers from modules in any case? In which case that should be done first as a separate PR?

OK. I'll make this one a draft and create 2 new ones.

@pmario pmario marked this pull request as draft October 31, 2023 22:27
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.

3 participants