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

MWS remove function wrapper #8624

Draft
wants to merge 1 commit into
base: multi-wiki-support
Choose a base branch
from

Conversation

pmario
Copy link
Member

@pmario pmario commented Sep 23, 2024

This PR removes the function-wrapper according to you comment at: #7596 (comment)

[. . .] 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?

We need to start somewhere. IMO MWS would be a good starting point to prepare for a future code formatting setup.

Copy link

stackblitz bot commented Sep 23, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

Confirmed: pmario has already signed the Contributor License Agreement (see contributing.md)

@pmario pmario changed the base branch from master to multi-wiki-support September 23, 2024 10:35
@Jermolene
Copy link
Member

Hi @pmario is the idea here to work through the plugins a few at a time as an experiment? I think if we're going to do this I wouldn't be against doing it globally all in one go. The tricky thing is identifying the modules that do actually need an anonymous function wrapper.

@pmario
Copy link
Member Author

pmario commented Sep 30, 2024

Yes. The idea is to start with the plugins, because they are easy to review and relatively easy to test on a per-plugin basis.

Once the plugins are done, we could do "the core" in one go.

I did a test-run more than a year ago. See: #7596 to test it with prettier-formatter. -- Now it will be more than 500 files, which imo makes it very hard to review.

Even if it only takes 30 seconds per file it will take a several hours to do them all.
Last time I was not very lucky with a global "find and replace". So I ended up doing search and replace per directory.

Once a directory was created, I did run the tests, which is about 60 seconds on Windows. It would be much faster on linux 3-5 seconds. If the tests passed I did start a Node.js version and had a manual look at the wiki.

So it will be at least 1 day of work with some testing. -- After the PR is created we need to be fast to merge it. Since every new merge to any js-code file will invalidate the PR and causes a lot more work to fix it again.

just my thoughts.

@Jermolene
Copy link
Member

Hi @pmario would it be possible to leave MWS until later? It is under active development with open pull requests, so it might be better to focus on the more dormant plugins first.

@pmario pmario marked this pull request as draft September 30, 2024 19:51
@pmario
Copy link
Member Author

pmario commented Sep 30, 2024

Sure. I will convert it to a draft

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.

2 participants