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

TypeScript support in core #4662

Merged
merged 32 commits into from
Sep 18, 2023
Merged

TypeScript support in core #4662

merged 32 commits into from
Sep 18, 2023

Conversation

sneridagh
Copy link
Member

@sneridagh sneridagh commented Apr 6, 2023

So, let's put this on the table and discuss it during the Beethoven Sprint.

To be clear: I'm (still) far from being sold on TypeScript, and have profound doubts on how it could benefit Volto in short/mid term (as I always did). For a number of reasons:

  • Plone/Volto developers base are not familiar with it,
  • let alone old Plone community members who wants to adopt Volto for their next projects.
  • The community lacks of "TypeScript Wizards(TM)" to rely on when things get weary.
  • Problems derived from adding an additional abstraction layer to the equation outweighs all the possible benefits that it could bring, and a whole new category of errors (typings) to solve, instead of focusing in features and getting things done.
  • "Type safety" is still a concept that, personally speaking, is overrated and often misunderstood by "hurray, my IDE autocompletes everything". Unit, acceptance testing and documentation are as important as "type safety".

However, once named the "cons", there are also some "pros".

  • Best way to document code internally, signatures, props, config object, etc.
  • Could be the entry point and the enabler of some other complex things.
  • The whole ecosystem is migrating to it, and the typings permeate the IDEs and your code.
  • Volto is becoming stable enough to start introducing it.

Summarising, the proposal is not to migrate Volto to TypeScript, but having the ability to use it here and there in core if appropriate in some specific areas to gain knowledge and confidence as a tool, and artefact, not as a must.

I know could be a drawback and might be chaotic to have some things in JS and others in TS. As said, let's discuss it during the sprint, and see how it goes.

/cc @mtoepfl I'd like to know your feedback about the PR, if you please could take a look, it would be great!

Regarding implementation details, I added a tsconfig.json, adding TypeScript support for .ts files only as Razzle already supports it. In the ESlint side, I replaced the (deprecated) babel-eslint with @babel/eslint-parser, and use the TypeScript ESlint parser only on the .ts files. So effectively this way Volto supports both JS and TS at the same time.

UPDATE: I fixed some ESlint issues that the new parser and rules was complaining about. The important changes related to this PR are in the root.

UPDATE (2023-04-17): I've managed to make the change non-breaking for 16 series: #4705 Undoubtedly, there could be backport issues still, but it's doable.

@sneridagh sneridagh requested review from davisagli, tiberiuichim, mtoepfl, pnicolli and a team April 6, 2023 19:29
@netlify
Copy link

netlify bot commented Apr 6, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 24f98cb
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65082d9935cc8500083ce6dd
😎 Deploy Preview https://deploy-preview-4662--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pnicolli
Copy link
Contributor

pnicolli commented Apr 6, 2023

😃

  • Plone/Volto developers base are not familiar with it,

Agreed that this is an issue, it is my main issue on this topic, the other ones are just smaller obstacles.

  • let alone old Plone community members who wants to adopt Volto for their next projects.

All of these people can use JavaScript in their projects as they would today. Adopting TS in core does not mean forcing people to adopt it for projects that use Volto. It would just improve their developer experience actually.

  • The community lacks of "TypeScript Wizards(TM)" to rely on when things get weary.

Well, we kind of often lacked in JavaScript experience in the community, and Volto is a simple enough case in my opinion for TypeScript not become a enormous problem. Also, typings can be rough and improved later, for possible complex situations.

  • Problems derived from adding an additional abstraction layer to the equation outweighs all the possible benefits that it could bring, and a whole new category of errors (typings) to solve, instead of focusing in features and getting things done.

I believe you might be underestimating the benefits here, with blocks, config and api being the most complex things to work with and the ones that would benefit the most from TS.

  • "Type safety" is still a concept that, personally speaking, is overrated and often misunderstood by "hurray, my IDE autocompletes everything". Unit, acceptance testing and documentation are as important as "type safety".

Yes, those things are crucial, TypeScript aims at being an addition to those, it is not there to allow us to remove tests or docs.
My preferred TS benefit is consistency in refactoring, something that has been done several times in this product.

However, once named the "cons", there are also some "pros".

  • Best way to document code internally, signatures, props, config object, etc.
  • Could be the entry point and the enabler of some other complex things.

Zod? 😃

  • The whole ecosystem is migrating to it, and the typings permeate the IDEs and your code.
  • Volto is becoming stable enough to start introducing it.

Actually it would have been more beneficial before being stable than after being stable, for the reasons explained above. But it's never too late, and Volto will never at perfect stability.

Summarising, the proposal is not to migrate Volto to TypeScript, but having the ability to use it here and there in core if appropriate in some specific areas to gain knowledge and confidence as a tool, and artefact, not as a must.

Imho, the goal should be to enable TS, yes, but with the long term dream to completely migrate, otherwise the benefits are cut in half (but still there).

I know could be a drawback and might be chaotic to have some things in JS and others in TS. As said, let's discuss it during the sprint, and see how it goes.

Regarding implementation details, I added a tsconfig.json, adding TypeScript support for .ts files only as Razzle already supports it. In the ESlint side, I replaced the (deprecated) babel-eslint with @babel/eslint-parser, and use the TypeScript ESlint parser only on the .ts files. So effectively this way Volto supports both JS and TS at the same time.

iirc this is basically the same we suggest in the docs, so it's perfectly fine by me

@mtoepfl
Copy link

mtoepfl commented Apr 6, 2023

Hey @sneridagh. Thanks for mentioning. I'm happy to read this message this evening.

I'm happy to revisit the changes I did on the sprints at the conference and push it to this branch.

I only had some problems with some export * from 'xxxx'; statements, but the rest looks really promising.

A problem is that you have to touch many files which are hard to merge if the modified files are changed by other pull requests. So a discussion/decision beforehand could save a lot of time.

I would love to answer questions on typings. Mostly it's very straight forward and easy to learn for app-parts. For some tweeks in "library"-parts, I'm sure, that there are many real "TypeScript Wizard" on the internet who will be happy to jump in and give feedback if needed.

I am interested to see the comments of the other core developers.

@mtoepfl
Copy link

mtoepfl commented Apr 6, 2023

@pnicolli:

My preferred TS benefit is consistency in refactoring, something

100% dito: eg. thinking of refactoring the fetch/redux logic

+1 for this and all other points you mentioned

@sneridagh
Copy link
Member Author

@mtoepfl I knew you'd like it ;) I'd love to have the work you did during the last PC sprint!

Btw, could we have it separated from this PR? So we can keep this one as the PR that enables the support, then all the other related work.

I'd like also to have full ok and support from the Volto Team and hear all the feedback about it before proceeding.

Regarding the export X from Y problem, it's fixed.

@cypress
Copy link

cypress bot commented Apr 6, 2023

Passing run #7177 ↗︎

0 577 20 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge branch 'master' into typescript-support-in-core
Project: Volto Commit: 87fe50464e
Status: Passed Duration: 13:43 💡
Started: Sep 12, 2023 11:59 AM Ended: Sep 12, 2023 12:13 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@sneridagh
Copy link
Member Author

@tiberiuichim if you have a moment, due to upgrades to the ESlint plugins, now react-hooks plugin is complaining about:

/Users/sneridagh/Development/plone/volto/src/components/manage/Blocks/Search/hocs/withQueryString.jsx
  18:11  warning  The 'indexes' logical expression could make the dependencies of useEffect Hook (at line 24) change on every render. Move it inside the useEffect callback. Alternatively, wrap the initialization of 'indexes' in its own useMemo() Hook  react-hooks/exhaustive-deps

✖ 1 problem (0 errors, 1 warning)

https://github.com/plone/volto/pull/4662/files#diff-fc8bdb4141e8455cee5f70633025f8027f1449a064b16bf87b3530d28ee2c0b2L18

Could you please take a look? Thanks!

@sneridagh
Copy link
Member Author

@plone/volto-team I just realised of one thing.

Since I had to update ESlint to the latests version and I changed the parser to use @babel/eslint-parser, and I've updated all the ESlint plugins, it must be a breaking one (since now it reports more things than before, affecting all projects out there.

The CI is green for a project (but we don't have any .ts component yet), so we don't know how this will affect them. We'll have to see, but I expect some adjustments there too (at least jsconfig->tsconfig) changes.

Marking it as breaking change shouldn't be any problem at first sight, but that means that we won't be able to port anything TS based to 16... which is an important drawback...

Something to talk about during the sprint.

* master: (22 commits)
  Release changelog notes for 16.20.1
  Release 17.0.0-alpha.5
  Generate a split sitemap (also fix robots.txt) (#4639)
  Fix search block in edit mode re-queries multiple blocks with an empty search text (#4694)
  Fix Move to top of folder ordering in folder content view (#4691)
  Changelog
  Revert "Add current page parameter to the route in the listing and search block pagination (#4159)" (#4695)
  Release generate-volto 7.0.0-alpha.4
  Force the resolution of the `react-error-overlay` package to `6.0.9` (#4687)
  Fix training links (#4635)
  Release 17.0.0-alpha.4
  Release changelog notes for 16.20.0 (#4684)
  Update to latest backend versions (#4682)
  Support RelationList field with StaticCatalogVocabulary and SelectWidget. (#4614)
  Load a theme via a `theme` key in `volto.config.js` or in `package.json` (#4625)
  docs: improve creating view documentation (#4636)
  fix sitemap.xml.gz is not compressed #4622 (v2) (#4663)
  Make URL a literal string to fix broken link (#4667)
  Move developer guidelines to contributing #4665 (#4666)
  Update Volto contributing to align with and refer to the new Plone co… (#4634)
  ...
@sneridagh
Copy link
Member Author

I've managed to make the change non-breaking for 16 series: #4705 Undoubtedly, there could be backport issues still, but it's doable.

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

We need docs for this feature addition. Something like:

We got TypeScript! w00t! 🥳 🎈 

But you don't *have* to use it, you knuckle-dragging mouth breathing luddites.

Oh, OK, maybe not quite like that (which would be worse than no docs), but I'm sure y'all can improve upon it. 😜

sneridagh and others added 3 commits May 20, 2023 17:43
* master: (29 commits)
  Remove max_line_length from .editorconfig (#4776)
  Fix bug showing logs at the browsers when richtext widget is use (#4780)
  Show expired and future content in contents view (#4764)
  Fix reducing expanders loaded in a subrequest (#4761)
  Update release notes for 16.20.5 and 16.20.6 (#4759)
  Release 17.0.0-alpha.7
  Try to sort out volto's use of language codes (#4741)
  Improve .npmignore to not include not needed files/folders (#4746)
  Release 17.0.0-alpha.6
  Control panel list SSR (#3749)
  Open all accordion'd content in InlineForm by default, allow arbitrarily close any number of them (#4178)
  Upgrade to Plone 6.0.4 (#4743)
  fix: unresponsive add page (#4507)
  Apply suggestion from browser for password field (#4524)
  (fix):Object.normaliseMail: Cannot read properties of null (#4558)
  Fix link in Volto, remove from linkcheck ignore in Documentation v6.0 (#4742)
  added documentation regarding the static middleware #4518 (#4736)
  Closes issue #4567 (#4570)
  Fix whitespace in locales created by the generator (#4737)
  Tidy up from synch with 16.x.x (#4728)
  ...
* master: (29 commits)
  Remove anonymous function calls. Remove default exports from. (#4917)
  Release 17.0.0-alpha.14
  Linked headlines (#3540)
  Release notes for 16.20.8 16.21.0 16.21.1 (#4910)
  Spanish translation (#4896)
  Refactor Anontools (#4845)
  Update to plone-backend 6.0.5 (#4897)
  Release 17.0.0-alpha.13
  Enforce max upload size (#4868)
  Fix and improve the `addStyling` helper (#4880)
  Release 17.0.0-alpha.12
  Fix regression in horizontal scroll in contents view, add it back (#4872)
  Configurable Container component from registry for some key route views. (#4871)
  Allow to deselect color in ColorPickerWidget. (#4839)
  Release 17.0.0-alpha.11
  Pagination with router params (#4698)
  Release 17.0.0-alpha.10
  feat(slate): Add css identifier to slate style menu options (#4847)
  Update Brazilian Portuguese translations (Fixes #4853)
  Convert header class to function (#4767)
  ...
* master:
  Add a marker in the props for `RenderBlocks` in case that we have a container (#4932)
  Update the 'version addded' information regarding the Grid block in docs
  Release 17.0.0-alpha.16
  Grid block in core + primitive Container block (#3180)
  Release 17.0.0-alpha.15
  Fix changelog message and docs for #4848 (#4927)
  Add Storybook story for useDetectClickOutside hook with several demos (#4923)
  Fix the experimental add new block button position, compensate the ic… (#4924)
  Use proper heading tag (depending on the headline) in default listing template (#4848)
  Fix Annontools storyBook (#4921)
Copy link
Contributor

@pnicolli pnicolli left a comment

Choose a reason for hiding this comment

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

Just a small comment, but LGTM.

tsconfig.json Outdated Show resolved Hide resolved
* master: (42 commits)
  make selectedView and className props available for Search block (#4997)
  Release @plone/volto-testing 4.0.0-alpha.0
  Release 17.0.0-alpha.21
  Upgrade to Cypress 12.17.1 (latest) (#4981)
  Image rendering (#3337)
  feat(Url.js): add getFieldURL helper function to get the url value of a field based on its structure (#4731)
  Handle @linkintegrity response with items but no breaches (#4832)
  Release 17.0.0-alpha.20
  Use all the apiExpanders in use, so we perform a single request for getting all the required data. (#4946)
  Fix the condition deciding on listing pagination format so it takes into account container blocks as well (#4978)
  Release 17.0.0-alpha.19
  Fix search block input clear button doesn't reset the search (#4837)
  Add /ok route as an express middleware (#4432)
  handles condition for yearly frequency in recurrence (#4604)
  Remove dangling out of place Guillotina Cypress tests (#4980)
  Update to latest plone.restapi and Plone 6.0.6 (#4979)
  Update browserlist (#4977)
  `Links and references` view via content menu [Add `Links to item` view] (#4787)
  Release 17.0.0-alpha.18
  Toc responsive (#4912)
  ...
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Minor English and MyST syntax, grammar, and style.

docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
sneridagh and others added 3 commits July 26, 2023 12:42
* master: (59 commits)
  (feat):Update toc block entries (#5146)
  view cypress test (#5149)
  fix search block search results number (#5171)
  fix : RecursiveWidget is incorrectly translated (#4513)
  Bugfix : Responsive Error in login page  (#4309)
  Fix default template for content type which contains block with schem… (#5159)
  Updated Spanish translation (#5120)
  ContentRules add and edit forms for languages other than English (#5162)
  Fix SelectWidget throwing error when editing a recently created conte… (#5155)
  Don't show ``No value`` option in SelectWidget and ArrayWidget if default value is 0 (#5152)
  Fix storybook config for project generator. Add support for SCSS, upgrade to webpack 5 in there as well. (#5132)
  Fix linkcheckbroken 301 redirect to https://www.dlr.de/de (#5131)
  Release 17.0.0-alpha.25
  fix: Fix querystringResults subrequests id, to work properly in dupli… (#5071)
  Fix accessibility of the content folder buttons (#5101)
  Refactor Comment (#4874)
  Refactor Search Tags (#4873)
  Logout Refactor (#4860)
  Add www.dlr.de to README (#5112)
  Fix default toc renderer for nested entries (#5116)
  ...
* master:
  Fix use of CSS modules in webpack 5 (#5165)
  fix back button on search block to update the search results for #4402 (#4879)
  Fix toc accessibility issue (#5058)
  Add external className to UniversalLink for external link. (#5110)
  Refactor Login (#4933)
  Refactor Messages (#4926)
  Refactor Navigation (#4876)
@sneridagh
Copy link
Member Author

sneridagh commented Sep 12, 2023

Decision Volto Team Meeting (2023-09-12): It will be included in Volto 17. However, it needs to be documented properly. @sneridagh commits himself to work on this during the PloneConf Sprint (and other documentation matters).

  • How to use React TypeScript
  • How to configure IDEs to support TypeScript
  • Which are the limits of usage (if any) and the expectations (eg. no code would be migrated, it won't be mandatory for any code).
  • How to handle backports to stable (if any).

@mtoepfl
Copy link

mtoepfl commented Sep 12, 2023

🎉 I'm going to join you at coming PloneConf Sprint.

I could try to cherry pick my changes of the PloneConf Sprint last year. Before I planned to write a benchmark for creating brains with many fields in metadata for ZCatalog. I feel it's getting over proportional slower in reading, when adding many fields to metadata. Maybe there ist a bottleneck. But finding this out should not take too long.

Is it possible to merge some of the refactoring pull requests to functional components before the Sprint?

@sneridagh
Copy link
Member Author

@mtoepfl awesome! I might multiplex this and docs this year.

Could be that your changes overlap with this PR I made: #4949

We will have to merge them both and you review my crappy TS ;)

I can't wait!

* master:
  Add external className to slate Link view. (#5188)
  Release generate-volto 7.0.0-alpha.6
  Fix addon i18n local command when executed outside the scope of a Volto project. (#5181)
  Release 17.0.0-alpha.26
  Use the @navroot and @site controlpanels to render the <title> and the logo (#3537)
  User / Groups search control panel / sharing panel improvements #4551 (#5053)
  User Control Panel improvements (#4572)
  Fix generator build
  Release generate-volto 7.0.0-alpha.5
  Add dockerized approach to add-on generator (#5167)
@sneridagh
Copy link
Member Author

@stevepiercy I made some additions to the docs, could you please take a look?

@pnicolli I tried this in existing projects and seems to work fine, even without changing the jsconfig->tsconfig so I guess we are good.

I updated the docs to reflect all that should be done in order to update a project to TS (which is acceptable).

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Mostly grammar and syntax fixes, but a question or two. This is great! Thank you!

docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/contributing/typescript.md Outdated Show resolved Hide resolved
docs/source/contributing/typescript.md Outdated Show resolved Hide resolved
docs/source/contributing/typescript.md Outdated Show resolved Hide resolved
docs/source/contributing/typescript.md Outdated Show resolved Hide resolved
docs/source/contributing/typescript.md Outdated Show resolved Hide resolved
docs/source/contributing/typescript.md Outdated Show resolved Hide resolved
docs/source/contributing/typescript.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
src/components/theme/TsTest/TsTest.test.tsx Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/contributing/typescript.md Outdated Show resolved Hide resolved
@sneridagh sneridagh merged commit 08e384b into master Sep 18, 2023
40 checks passed
@sneridagh sneridagh deleted the typescript-support-in-core branch September 18, 2023 11:45
erral pushed a commit that referenced this pull request Sep 19, 2023
Co-authored-by: Nina <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Piero Nicolli <[email protected]>
sneridagh added a commit that referenced this pull request Sep 19, 2023
* master: (31 commits)
  Refactor Content Rename Model (#4971)
  Refactor Content Tags Modal  (#4970)
  Refactor Content workflow Modal  (#4969)
  Refactor Content Property Model  (#4968)
  Refactor Sidebar (#4965)
  Refactor PersonalTool component (#4954)
  Fix standalone navigation action call if expander is set (#5197)
  Fix api convenience buildout build
  Fix instruction to fetch add-on from repo (#5196)
  Remove JSON files from being linted by ESlint, since it's not its pur… (#5194)
  Release generate-volto 7.0.0-alpha.8
  Add missing empty lock to acceptance generator addon folder (#5193)
  Release generate-volto 7.0.0-alpha.7
  Release 17.0.0-alpha.27
  TypeScript support in core (#4662)
  Update delete content modal for link integrity (#4786)
  Add external className to slate Link view. (#5188)
  Release generate-volto 7.0.0-alpha.6
  Fix addon i18n local command when executed outside the scope of a Volto project. (#5181)
  Release 17.0.0-alpha.26
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants