[RFC] TS migration patterns #1732
Conversation
Renames index.es.js to .ts and adjusts main entry point. Disable all checks initially so we can clean them up one by one. Additionally, adds ts-loader to webpack config and teaches it to handle TypeScript files with priority over JS ones.
This is going to be arguably the hardest thing to get right in our migration to TypeScript. The `Liferay` object is global and augmented from way too many different sources making it really hard to type. This commit aims to start simple and get us to a point where we can start adding more types as we go. For now, it simply adds the `Liferay` object and describes all the properties defined in `portal-web/top_js.jspf`. It is left for future commits to: - Refine existing types to better match the `Liferay` object - Add more types as needed - Figure out a better way to handle the `Liferay` object
- Converts autosize to ts - Converts html_util to ts because it is a dependency of autosize - Makes dependency explicit rather than implicit via the `Liferay` global With the last change, we avoid having to add `Liferay.Util.unescapeHTML` to the ILiferay interface coming from `global.es.js`. This should be the preferred approach. Consuming the proper APIs rather than reaching out to the global namespace. In `html_utils`, MAP_HTML_CHARS_ESCAPED and MAP_HTML_CHARS_UNESCAPED are somewhat badly typed. Their indexed access type should probably return `string | null`. Internally, it will never get accessed with a non-valid key, so it should be safe to assume that the type is `string` to simplify things for now.
I picked the lowest hanging modules I could find to get them out of the way. One interesting thing to note is that we have some argument type guards in place. These usually take the form of `if (typeof arg !== <type>)`. We usually check and throw an Error during runtime if the types don't match. This should no longer be necessary if we could guarantee all of our consumers were using TS. Unfortunately, that's not really the case, so the guards can still play a role albeit with diminishing returns. A side effect of this is that when we're testing the guard, TS will complain automatically about us passing the wrong type. That is, if we try to pass an object to a method that expects a number, TS will complain about it. The solution picked here is to use `as any` to cast the argument to get it through.
The most interesting part in this conversion is the code change to use `instanceof` instead of a custom if clause to guard the type and narrow down the url argument to the correct type. This is a rather common pattern in Liferay. Accepting several different arguments and then using the same variable indistinctly after some normalization steps. A possible alternative would be to create a real "normalizer" function `(url: string | URL) => string` that would take the url and return a normalized version with the type narrowed down to `string`.
Interesting note here though is the fact that TS caught a bug in our test or potentially in our implementation of `addParams`. The implementation checks wether the first argument is `string | object`. The test goes on to put that to the test passing an array which, coincidentally is an object as far as `typeof` is concerned. The test expects a TypeError to be thrown which funny enough, is thrown because the call is missing the second argument. I've decided to remove the test assertion since passing an array as params sounds like a valid use case to me even though the keys would be the number indexes.
This conversion is pretty straightforward but showcases some interesting
things to consider:
1. Mocking the Liferay global object
Since we declare `Liferay` as a global var, mocking it is not straightforward.
- Partial mocks are flagged as TS as incompatible
- Reassignments in setup or teardown methods won't get picked up by TS
forcing us to cast to `Jest.Mock` and/or other types
For the sake of progress I simply resorted to "as any" here, but maybe we
should consider a better approach. Some possible options include:
- Simply export the type and locally cast usages of the global API
instead of defining a global var
- Further split the definition so we can use Partial<ILiferay> and
Partial<IThemeDisplay> to construct partial mocks
2. String-indexed objects
We do a lot of willy-nilly object string-index access in our codebase. Maybe
as we progress our types will become more explicit, but for now it seems
like we want the type `{ [key: string]: string }` more often than not.
- Does this type have a common name?
- Should we export this type for general consumption or define it locally?
3. To TS or not to TS
This stems a bit from the previous point and connects with prior commits
where we addressed the fact that we keep runtime type guards in our code.
In this case, we consume `getPortletNamespace(portletID: string): string`,
but do so trying to pass `portletID: string | null`.
This hints that we might have cases of `createPortletURL` throwing exceptions
in runtime through the `getPortletNamespace` utility. Again, in the spirit of
progress, I just added a non-null assertion here `portletID!` which we know
is obviously wrong.
Updating the definition to `getPortletNamespace(portledID: string | null)`
seems to be technically correct but pushes the problem down the system. We
likely don't want a system where `null`s are being passed around unknowingly.
I'm not sure what the best approach is here, so just writing this down here
to see how this goes, specially as we move onto other modules.
Without this, TS sees `portletID: String | null`, probably because it can't figure out that the 2 disjoint conditions over `params.length` force `portletID` to actually be just `String`.
Disabled temporarily @liferay/portal/no-global-fetch because a valid import depends on the `.es.js` extension which is no longer the case. Will need to fix `eslint-config/.../no-global-fetch.js` before merging
This is obviously not a TS migration :) The `CompatibilityEventProxy` class added compatibility between YUI and metal events, re-emitting events according to YUI naming and adding the capability of adding targets to bubble events to them. Only usage I'm aware of was in an old version of TranslationManager which was already updated as part of [Update TranslationManager component](https://issues.liferay.com/browse/LPS-103180) which initially removed the dependency at liferay@a4336c0
- [HTMLCollection should be string indexable](microsoft/TypeScript#19437). But it isn't, so `form.elements[elementName: string]` is a no go. I decided to go with `HTMLFormControlsCollection.namedItem()` instead which should do about the same. - Improves `isObject` help the compiler infer the type resulting after it by returning a non-nullable version of same type passed to it. However, it won't return `never` for `isObject(foo: string | boolean | number)` so it can still use some further refinement.
…_dom utilities to TS
|
CI is automatically triggering the following test suites:
|
liferay-continuous-integration
added
ci:test:sf - failure
and removed
ci:test:sf - pending
labels
❌ ci:test:sf - 0 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: ts-migration-patterns 1 Failed Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#1350 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#1732 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - kresimir-coko > liferay-frontend - PR#1732 - 2021-12-08[03:06:59] Testray Importer:publish-testray-report#4576 |
liferay-continuous-integration
added
ci:test:relevant - failure
and removed
ci:test:relevant - pending
labels
❌ ci:test:relevant - 0 out of 1 jobs passed in 17 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 15900daffaa79a8e8c5e87a6808001c159aa7ad0 ci:test:relevant - 0 out of 1 jobs PASSED1 Failed Jobs:For more details click here.Failures unique to this pull:
For upstream results, click here.Test bundle downloads:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#1458 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#1732 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - kresimir-coko > liferay-frontend - PR#1732 - 2021-12-08[03:06:59] Testray Importer:publish-testray-report#4577 |
| @@ -1251,36 +1405,37 @@ describe('App', function () { | |||
| userEvent.click(link, {button: 1}); | |||
| userEvent.click(link, {button: 2}); | |||
There was a problem hiding this comment.
These lines 1401-1406 contain a 2nd parameter, but the userEvent.click() expects 1 parameter instead of 2.
What do we do here, can I safely delete these because it's supposed to not work as the method doesn't support the 2nd parameter? Do we leave it as is and do // @ts-ignore?
Alternatively, I can leave this one until the end until it all works and then see if this test fails without those 2nd parameters.
| app = new App(); | ||
| app.addRoutes(new Route('/path1', NullStateScreen)); | ||
| app.navigate('/path1').then(() => { | ||
| app?.navigate('/path1#hash').then(() => { | ||
| window.addEventListener( | ||
| 'popstate', | ||
| () => { |
There was a problem hiding this comment.
expect(getCurrentBrowserPath(document.referrer)) expects 0 argument but received document.referrer. Similar to another comment here, do we just straight up delete document.referrer as it "probably" doesn't do anything here?
| @@ -1770,7 +1972,8 @@ describe('App', function () { | |||
| StubScreen2.prototype.activate, | |||
| StubScreen.prototype.disposeInternal, | |||
| ]; | |||
| for (var i = 1; i < lifecycleOrder.length - 1; i++) { | |||
|
|
|||
| for (let i = 1; i < lifecycleOrder.length - 1; i++) { | |||
| expect( | |||
| lifecycleOrder[i - 1].mock.invocationCallOrder[0] | |||
There was a problem hiding this comment.
Property 'mock' does not exist on type '() => void'
Not sure what's happening here, I assume that this is another bug that just worked without static types.
| exitDocumentLinkElement(); | ||
|
|
||
| return super.evaluateStyles(surfaces); | ||
| return super.evaluateStyles(); | ||
| } | ||
|
|
||
| evaluateScripts(surfaces) { |
There was a problem hiding this comment.
surfaces has typing issues, setting it to surfaces: Surfaces throws an error on the evaluateScripts function:
Property 'evaluateScripts' in type 'TestScreen' is not assignable to the same property in base type 'Screen'.
Type '(surfaces: Surface) => Promise<void>' is not assignable to type '(surfaces: Surfaces) => Promise<void>'.
Types of parameters 'surfaces' and 'surfaces' are incompatible.
Type 'Surfaces' is missing the following properties from type 'Surface': activeChild, defaultChild, element, id, and 18 more.ts(2416)
Also Property 'scheduledNavigationEvent' does not exist on type 'App'. Did you mean 'scheduledNavigationQueue'?... Not sure if that's a preexisting bug or what, very confusing.
Both of these errors are repeating on other mentions of that same code further along in the file.
| .prefetch('/path') | ||
| app = new App(); | ||
| app.addRoutes(new Route('/path', HtmlScreen)); | ||
| app.prefetch('/path') | ||
| .then(() => done.fail()) | ||
| .catch(() => { | ||
| expect(fetch.mock.calls.length).toBe(0); |
There was a problem hiding this comment.
Property 'mock' does not exist on type '(input: RequestInfo, init?: RequestInit | undefined) => Promise<Response>'
🤷 haven't looked much into this one, but might be a limitation of our current testing architecture
| return new Promise((resolve) => { | ||
| window.addEventListener( | ||
| 'popstate', | ||
| () => { | ||
| expect(this.app.reloadPage).not.toHaveBeenCalled(); | ||
| expect(app?.reloadPage).not.toHaveBeenCalled(); | ||
|
|
||
| resolve(); | ||
| }, |
There was a problem hiding this comment.
Expected 1 arguments, but got 0. Did you forget to include 'void' in your type argument to 'Promise'?
resolve() expects an argument, not sure what to give it here.
| resolve(); | ||
| }, | ||
| {once: true} | ||
| ); | ||
| this.app.skipLoadPopstate = true; | ||
|
|
||
| app.skipLoadPopstate = true; |
There was a problem hiding this comment.
As is, app.skipLoadPopstate = true;, complains that app is possibly null. When I add app?... it complains about the entire line, saying The left-hand side of an assignment expression may not be an optional property access..
Googling for this tells me to edit the interface of the App and add an optional property to skipLoadPopstate. Do we wanna do stuff like that in the future?
| } | ||
|
|
||
| function preventDefault(event) { | ||
| function preventDefault(event: Event) { | ||
| event.preventDefault(); | ||
| } | ||
|
|
There was a problem hiding this comment.
On line 2511 (can't comment on it in this UI because it has no changes) the whole window.location thingy is smelly. TS throws an error The operand of a 'delete' operator must be optional. when we want to delete it, and also it complains about mismatching properties on the line below when we want to assign new properties to it saying Type '{}' is missing the following properties from type 'Location': ancestorOrigins, hash, host, hostname, and 9 more.
|
Okay so I started to take a look but found myself asking the question "what is the primary goal of the PR" here. So, I am taking a step back here from the code itself I think we need to ask ourselves what our goal is. I found two TS related issues we have current, LPS-135334 & LPS-131257 If we are just thinking in terms of "Migrate to TS", it is going to be a pretty large umbrella and it will be easy to have scope creep, which will cause this task to drag on for awhile. Additionally, if we try to complete this by chasing TS errors, I think we are going to have a tough time and will drag on forever. So I think it would be helpful to take a step back first and break this up into smaller and more manageable goals. I think our primary goal right now is getting type support for For
Option # 1 is going to be the most tedious and has potential for regressions since we would likely need to re-write some code. Option # 2 will be quicker to implement but also annoying because anytime we update our source code, we also must manually update the types file. Personally, I think we should go with option # 2 and create our own definition file from scratch because we can finish this more quickly and also not have to touch our source code. And since we don't update this module that often, I think we can deal with the set back of manually updating type files. Now, for the global For example: Types for So, how do we actually do this? I think to properly move forward we sort of need to take a step back and tackle this in steps. I would probably encourage just starting an entirely new branch and then consult back to Chema's PR when necessary. And I wouldn't really worry about anything outside of Steps:
@kresimir-coko, how does that sound to you? I didn't get too granular in the specifics of the code, because I don't know if that is really helpful at this point because we haven't quite defined our goal yet. Hopefully I didn't confuse you too much with my train of thought here. Lastly, for these remaining modules, they all depend on frontend-js-web or
Footnotes
|
|
I also created this codesandbox to give a smaller idea of how the files are constructed, https://codesandbox.io/s/fragrant-butterfly-h4q10?file=/modules/consumer/index.ts |
|
Thank you @bryceosterhaus, this is the kind of guidance that I needed when I started with this 🙏 I think I understand the approach we wanna go for now, and it makes a lot of sense. I'm really excited to dig into TypeScript and bring about a new era of frontend architecture. Who knows, we might get a mention in #lr-shoutouts one day. |
Chema agrees!
|
|
🥲 |
|
Closed in favor of a simpler approach in #1747 |
This is a continuation of Chema's #1379, I haven't changed anything fundamentally but only added on more accurate types.
The process
The first step was to deploy and source-format to see how it behaves, which produced mixed results, some of the 3 changed modules deploying, some not, some successfully getting formatted and
js-state-webnot. This step was preliminary to see where we're at and doesn't impact the rest of the PR in any significant way.This step was useful to gauge how many errors (didn't fit the terminal buffer) TS was producing, which was what drove me to start addressing them in the next step.
After that, I started following VSC's squiggly underlines to see which types need to be changed inside
js-spa-web.Notes
js-spa-web) that were easy and/or straight-forward to replaceWhy did I open this PR now
Hey @bryceosterhaus 👋 this is probably you reading.
This PR is here so that you, and whoever else wants to, can take a look at how I'm progressing with adding these types and to let me know if I'm on the right track.
My goal is to fix all TS errors in these 3 modules so they can properly compile/deploy.