[RFC] TS migration patterns #1379

jbalsas · 2021-08-20T09:38:35Z

This is an initial exploration of what a migration to TS would look like for some of our modules and how ready our infrastructure is for other teams to safely adopt TS in their own modules.

I'm sharing so we can discuss about the patterns and possible impediments we have to fully transition to TS and make sure our infra is solid for other teams to move onto it soon.

Each commit message has some information about specific topics that might be worth discussing, and I'll be highlighting some things in particular, but feel free to review this at your will and raise any questions you might think of.

@bryceosterhaus, @matuzalemsteles, @izaera, mentioning you in particular since you might have the most experience in TS codebases... did we ever codify some guidelines or best practices in Clay or the other repos that we used to test TS with? Any hints to share? Thoughts? 👼

liferay-continuous-integration · 2021-08-20T09:38:39Z

CI is automatically triggering the following test suites:

ci:test:relevant
ci:test:sf

jbalsas · 2021-08-20T09:40:13Z

modules/apps/frontend-js/frontend-js-spa-web/src/main/resources/META-INF/resources/init.ts

+// Properties from `frontend-js-spa-web/init`
+declare global {
+	export interface ILiferay {
+		SPA?: {


Moved here, this could simply be SPA?: typeof App

Yeah, I think it would be better because eventually we can modify the App and forget to update the types here.

jbalsas · 2021-08-20T09:40:31Z

modules/apps/frontend-js/frontend-js-state-web/src/main/resources/META-INF/resources/State.ts

@@ -16,6 +16,7 @@ import SubscriberMap from './SubscriberMap';
 import deepFreeze from './deepFreeze';

 import type {Immutable} from './types';
+import * as Types from 'frontend-js-web/src/main/resources/META-INF/resources/liferay/types';


Not loving this 😂

It would be great to add global types in a custom folder to be added via typeRoots to avoid importing types explicitly.

I actually prefer having to import types explicitly instead of them being globally available everywhere. I think this would also make sense for java devs to see types imported as well.

Plus, you can explicitly import types with ts import type {Liferay} from '/some-path/here';

Hmm to some extent I agree with that but it's a little weird because you wouldn't be using Liferay directly because they are declared globally, that's why I find it weirder because normally this behavior is so that it's visible to all files because we are adding to window, opposed to defining the specific types of a module. For example, it might make sense if it had a syntax like:

import type '/path/types';

Maybe if the types for Liferay weren't declare global but that wouldn't work. Either way it would be the same thing as the global types for Window.

Maybe if the types for Liferay weren't declare global

Yeah I think in an ideal world we wouldn't use Liferay as a global object and slowly more our use of it to be importable. But that is probably a long way off

liferay-continuous-integration · 2021-08-20T09:41:09Z

❌ ci:test:sf - 0 out of 1 jobs passed in 2 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: ce10eb3c2e2f3427b904c078ac8192ca5b7e8d37

Sender Branch:

Branch Name: types
Branch GIT ID: 0e56c3174113dec4787dd2528d37c4038f4bec6f

0 out of 1jobs PASSED

1 Failed Jobs:

~~test-portal-source-format~~

For more details click here.

     [exec]   21:1  error  imports must be sorted by module name (expected: "@liferay/frontend-js-state-web/src/main/resources/META-INF/resources/State" << "frontend-js-spa-web/src/main/resources/META-INF/resources/init")  @liferay/liferay/sort-imports
     [exec]   21:13  error  'SPATypes' is defined but never used. Allowed unused vars must match /^_/u.  @typescript-eslint/no-unused-vars
     [exec]   22:13  error  'StateTypes' is defined but never used. Allowed unused vars must match /^_/u.  @typescript-eslint/no-unused-vars
     [exec] 
     [exec] ??? 9 problems (9 errors, 0 warnings)
     [exec]   5 errors and 0 warnings potentially fixable with the `--fix` option.
     [exec] 
     [exec] 3 of 3 jobs failed
     [exec] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
     [exec] error Command failed with exit code 1.
     [exec] 
     [exec] > Task :packageRunCheckFormat FAILED
     [exec] Gradle build finished at 2021-08-20 09:40:57.420.
     [exec] 3 actionable tasks: 2 executed, 1 up-to-date
     [exec] 
     [exec] See the profiling report at: file:///opt/dev/projects/github/liferay-portal/build/reports/profile/profile-2021-08-20-02-40-38.html
     [exec] A fine-grained performance profile is available: use the --scan option.
     [exec] 
     [exec] 
     [exec] FAILURE: Build failed with an exception.
     [exec] 
     [exec] * What went wrong:
     [exec] Execution failed for task ':packageRunCheckFormat'.
     [exec] > Process 'command '/opt/dev/projects/github/liferay-portal/build/node/bin/node'' finished with non-zero exit value 1
     [exec] 
     [exec] * Try:
     [exec] Run with --info or --debug option to get more log output. Run with --scan to get full insights.
     [exec] 
     [exec] * Exception is:
     [exec] org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':packageRunCheckFormat'.
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.lambda\$executeIfValid\$1(ExecuteActionsTaskExecuter.java:208)
     [exec] 	at org.gradle.internal.Try\$Failure.ifSuccessfulOrElse(Try.java:263)
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeIfValid(ExecuteActionsTaskExecuter.java:206)
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:187)

liferay-continuous-integration · 2021-08-20T09:41:58Z

Jenkins Build:test-portal-source-format#575

Jenkins Report:jenkins-report.html

Jenkins Suite:sf

Pull Request:liferay-frontend#1379

Testray Routine:EE Pull Request

Testray Build:[master] ci:test:sf - jbalsas > liferay-frontend - PR#1379 - 2021-08-20[02:38:51]

Testray Importer:publish-testray-report#1817

jbalsas · 2021-08-20T09:42:49Z

...frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/add_params.ts

 */
-export default function addParams(params, baseUrl) {
+export default function addParams(params: string | object, baseUrl: string) {


We most certainly don't want object here but something like { [key:string]: string }... does that have a universal name?

Also... see comments about runtime type guards in the commits... keep'em or drop'em?

Additional future warning... typeof params !== 'object' might not be telling you what you thing is telling you... maybe we should lint about this or something, idk!

{ [key:string]: string }... does that have a universal name ?

Nope, but we can create one if it is very common...

I usually create types for this kind of option arguments. So something like this I think it's possible:

addParamsOptions { knownOption1: string; knownOption2: boolean; [key:string]: string; }

If there are no known options just the [key:string]: string; part. But having it as a named (and exported) type is usually handy when you need to pass those options through several function calls and have them typed.

Of course I'm not saying 100% of the time you must do this, but if it is a public API there are many benefits in having a named type for this, IMO.

If we don't have known properties on the object, instead of using [key:string]: string; we have the Record that was introduced in TypeScript, we can replace it with Record<string, string>;

we can replace it with Record<string, string>
+1

I prefer we don't create a global [key:string]: string; helper, as I think it may lead people to abuse it when they don't want to define a proper interface.

we can replace it with Record<string, string>
+1

Nice! Completely missed that! Works like a charm! ⭐

I prefer we don't create a global [key:string]: string; helper, as I think it may lead people to abuse it when they don't want to define a proper interface.

This is likely to happen anyways, isn't it? They can even use any 😂 . But yeah, totally get your point, let's keep those to a minimum until we know what's really useful and valuable! 👍

@jbalsas yeah they will still do it, but at least its not an encouraged type by us creating a unique helper for me.

no-explicit-any 🤔

jbalsas · 2021-08-20T09:44:13Z

...tend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/build_fragment.ts

 	const div = document.createElement('div');

 	div.innerHTML = `<br>${htmlString}`;

-	div.removeChild(div.firstChild);
+	div.removeChild(div.firstChild!);


Not sure if there's a better way... plaguing our codebase with non-null assertions doesn't feel right, yet it's certainly a powerful tool 😉

Yeah, I agree, as it is explicit above that we have a children I think this makes it safe

plaguing our codebase with non-null assertions doesn't feel right

I agree that it doesn't feel right to use these everywhere. If we do use it, I would prefer we have an explicit comment for why we are using it. So for this case, we can comment saying the <br> guarantees that there is a first node.

jbalsas · 2021-08-20T09:45:32Z

.../frontend-js-web/src/main/resources/META-INF/resources/liferay/util/get_portlet_namespace.ts

 */
-export default function getPortletNamespace(portletId) {
+export default function getPortletNamespace(portletId: string) {


Technically, this takes any... so... runtime type guards, keep'em or drop'em? Should we be stricter with types like this, or better type as our current usage and then clean it up?

runtime type guards, keep'em or drop'em?

I would say you still need them in case someone calls you from plain JavaScript...

However, if we decide to do runtime type checks, we should create a small utility for that, so that code is cleaner and less error prone. Something like:

Ensure.isString(portletId);

That we put at the top of methods.

runtime type guards, keep'em or drop'em?

I vote for keeping them since, like @izaera mentioned, you can still run this from plain js.

I would say you still need them in case someone calls you from plain JavaScript...

The thing that worries me here is that we're throwing TypeError exceptions on a bad call. It's not like we're gracefully putting the system in a place where it can recover, but rather crashing it to the ground at runtime.

This case is particularly tricky because the main difference is that if a developer calls this like getPortletNamespace(3) it will get:

// With runtime type guards Uncaught TypeError: portletId must be a string // Without _3_

At runtime, the uncaught error seems more harmful, although the silent valid return can let issues go unnoticed to the developer for a while.

The more common case I guess would be something like addParams(3, 'http://www.liferay.com')

// With runtime type guards Uncaught TypeError: Parameter params must be an object or string // Without Uncaught TypeError: params.trim is not a function

Now, I'm not convinced one way or another, so just trying to keep the conversation going here.

It's not like we have so many of these anyways, but maybe we could keep the runtime type checks for utilities that get exported globally (ie: accessible through Liferay.foo) but not for those that need you to import {foo} from 'frontend-js-web', assuming the latter will be more easily moved into TS

I agree that runtime failures are nasty, but people can avoid them simply by using a linter or TS at build time. If they don't want to do that, then they are exposed to more frequent runtime failures, because that's one of the reasons why typed languages were invented 🤷 .

Also, not throwing is much worse because you are deferring the failure to -basically- a random point in the future or, even worse, let it fail silently in ways that are very hard to reason about.

Say the developer writes addParams(3, 'http://www.liferay.com') and, instead of failing with Uncaught TypeError: params.trim is not a function, the code simply ignores the second parameter (because it uses an Object.keys or something like that, instead of trim).

Then, the method invocation would completely ignore the 'http://www.liferay.com' but the developer would be quite puzzled because all he sees is that he's passing the option (correctly, he thinks) but the method is not honoring it. How long will it take to realize what is the error?

And in production, this will lead to some kind of functional failure which users will suffer too, so, is that better than failing fast with an exception? I doubt it...

With TS we can still get runtime errors

Elm has entered the chat

I really don't want to make a distinction between people who choose TS and those who choose JS

It's not a distinction between TS and JS but between having a build with type checking or no build at all. Wether that type checking build is implemented with TS, eslint, or any other tool is an implementation detail.

Regarding runtime checks, I'm a big fan of super-robust public APIs for typed languages as well as dynamic ones, so I would make them (and probably design some way to make them automatic, not hand made), as I said above 🤷

It's not a distinction between TS and JS

I was referring to this part 😉

... people using plain JS are on their own (and should suffer, at least a bit, for choosing not to use types).

But, I understand what you said about runtime checks

(and should suffer, at least a bit, for choosing not to use types)

jbalsas · 2021-08-20T09:49:46Z

...s/frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/navigate.ts

+	url: string | URL,
+	listeners?: {[key: string]: Callback}
+) {
+	url = url instanceof URL ? String(url) : url;


We do a lot of type narrowing that TS can't pick up like in here. In this case, instanceof sounds like a better approach overall, but wondering what we do in general.

We're likely going to need to rewrite code because this code style is not easily expressed with types (that I could figure). There are other ways to guard types like user-defined or creating intermediate helper methods like getURLString(url: string | URL): string that help TS here.

Wondering how this will shape out in some of the more complex isomorphisms we have 😈

I have a similar in FilePath I think...

I use the AnyPath type for string | FilePath and FilePath.coerce to do the cast.

jbalsas · 2021-08-20T09:50:40Z

...-js-web/src/main/resources/META-INF/resources/liferay/util/portlet_url/create_portlet_url.ts

@@ -95,7 +94,7 @@ export default function createPortletURL(basePortletURL, parameters = {}) {
 	let namespace = '';

 	if (Object.entries(parameters).length) {
-		namespace = getPortletNamespace(portletID);
+		namespace = getPortletNamespace(portletID!);


For sure portletID can be null here. That's a possible bug somewhere that we haven't yet seen. Thoughts on what should we really do here?

Few lines above:

if (Object.entries(parameters).length && !portletID) { throw new TypeError( 'Portlet ID must not be null if parameters are provided' ); }

Generally speaking, I guess we could replace all manual type checks with TS. Unless we want them in runtime, to show some legitimate case in an error log 🤔

~~So that should be a || 😂 ?~~

~~We can add this one to the list of TS wins... doubt we would've ever seen that otherwise (other than with a bug report).~~

You see, the thing is that if parameters aren't provided, then portletID can be null, but then getPortletNamespace takes (or could take) non-nullable strings. That's the thing I wanted to raise the attention to. Need to decide how much we lean on the type system. I think the more the better, but maybe in this case null is a valid thing to pass to getPortletNamespace which doesn't seem to be the case since it will throw a TypeError in that case.

If parameters are not provided, we never reach getPortletNamespace:

if (Object.entries(parameters).length) { namespace = getPortletNamespace(portletID); }

In this case, the value of portletID, that we maybe took from basePortletURL, does not matter.

A bug here might be that we are not checking and namespacing params in basePortletURL. But, I'm guessing we are assuming the basePortletURL will be used as intended, a base URL with no portlet-specific params.

If parameters are not provided, we never reach getPortletNamespace:

lol, you're totally right... need to get me some better reading glasses 👴

I wonder why TS is unable to infer that portletID can't be null at that point. Maybe this could be written in a way that it would make it possible similar to what happens in #1379 (comment)

For this case I'll simply as string, then.

Maybe putting everything inside one if...

let namespace = ''; if (Object.entries(parameters).length) { if (!portletID) { throw new TypeError( 'Portlet ID must not be null if parameters are provided' ); } namespace = getPortletNamespace(portletID); }

Yeah, that should do it!

jbalsas · 2021-08-20T09:51:57Z

modules/apps/frontend-js/frontend-js-web/test/liferay/util/add_params.ts

-		expect(() => addParams(1)).toThrow(TypeError);
-
-		expect(() => addParams(['one', 'two'])).toThrow(TypeError);
+		expect(() => addParams(1 as any, sampleUrl)).toThrow(TypeError);


Another annoyance of keeping runtime type guards around... in tests, to test them you need foo as any :)

Funny enough, the second test was passing because the implementation throws TypeError from 2 different code branches, not because typeof [] !== 'object' 😉

I wonder if we can just remove these runtime tests because we are essentially already testing types via ts compiler. 🤷

I think that if we keep the runtime typechecks, then we'd likely want to keep the tests as well?

#1379 (comment)

Following the same reasoning, there's no guarantee people will call this from TS so chances are we won't be testing types?

Yep, that makes sense. If we have code that asserts a certain runtime error, we should test against it. 🏆

jbalsas · 2021-08-20T09:52:39Z

modules/apps/frontend-js/frontend-js-web/test/liferay/util/portlet_url/create_action_url.ts


 describe('Liferay.Util.PortletURL.createActionURL', () => {
 	it('returns a URL object with a href parameter containing the p_p_lifecycle parameter set to 1', () => {
 		Liferay = {
 			ThemeDisplay: {
 				getPortalURL: jest.fn(() => 'http://localhost:8080'),
 			},
-		};
+		} as any;


Mocking can get tricky... 🤔

Yes. But that's expected. That's why you have to use libraries like mockito in Java, too 🤷

Maybe using as const can be more secure, at least we can have the viability of what we have inside the Liferay mock.

jbalsas · 2021-08-20T09:53:08Z

ci:stop

liferay-continuous-integration · 2021-08-20T09:53:48Z

❌ ci:test:relevant - 0 out of 1 jobs passed in 15 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: ce10eb3c2e2f3427b904c078ac8192ca5b7e8d37

Upstream Comparison:

Branch GIT ID: ce10eb3c2e2f3427b904c078ac8192ca5b7e8d37
Jenkins Build URL: Acceptance Upstream DXP (master) #2232

ci:test:relevant - 0 out of 1 jobs PASSED

1 Failed Jobs:

~~test-portal-acceptance-pullrequest(master)~~

For more details click here.

Failures unique to this pull:

test-portal-acceptance-pullrequest(master)
Job Results:
0 Jobs Passed.
1 Job Failed.
```
Build was aborted
```

For upstream results, click here.

Test bundle downloads:

liferay-portal-bundle-tomcat.tar.gz (mirrors: dlc, lax)

liferay-continuous-integration · 2021-08-20T09:57:58Z

Jenkins Build:test-portal-acceptance-pullrequest(master)#669

Jenkins Report:jenkins-report.html

Jenkins Suite:relevant

Pull Request:liferay-frontend#1379

Testray Routine:EE Pull Request

Testray Build:[master] ci:test:relevant - jbalsas > liferay-frontend - PR#1379 - 2021-08-20[02:38:48]

Testray Importer:publish-testray-report#1819

izaera · 2021-08-20T10:29:39Z

...s/apps/frontend-js/frontend-js-react-web/src/main/resources/META-INF/resources/js/render.tsx

@@ -47,10 +47,10 @@ export default function render(
 	},
 	container: Element
 ) {
-	if (!(window.Liferay as any).SPA || (window.Liferay as any).SPA.app) {
+	if (!Liferay.SPA || Liferay.SPA.app) {


We should definitely provide typings for Liferay so that people can use it as documentation 👍

When I did the sample Angular project for the JS Toolkit, I had to use declare Liferay: any because we were lacking that, but it was clear that publishing the types for our Liferay interface would be a win (at least for the public parts).

izaera · 2021-08-20T10:30:13Z

modules/apps/frontend-js/frontend-js-spa-web/src/main/resources/META-INF/resources/init.ts

@@ -17,6 +17,8 @@ import ActionURLScreen from './screen/ActionURLScreen';
 import RenderURLScreen from './screen/RenderURLScreen';
 import {getUrlPath} from './util/utils';

+import * as Types from 'frontend-js-web/src/main/resources/META-INF/resources/liferay/types';


izaera · 2021-08-20T10:31:40Z

modules/apps/frontend-js/frontend-js-state-web/src/main/resources/META-INF/resources/State.ts

@@ -16,6 +16,7 @@ import SubscriberMap from './SubscriberMap';
 import deepFreeze from './deepFreeze';

 import type {Immutable} from './types';
+import * as Types from 'frontend-js-web/src/main/resources/META-INF/resources/liferay/types';


izaera · 2021-08-20T10:32:52Z

modules/apps/frontend-js/frontend-js-state-web/src/main/resources/META-INF/resources/State.ts

-			State: typeof State;
-		};
+	export interface ILiferay {
+		State?: typeof State;


How does this work? Is it merged with SPA in https://github.com/liferay-frontend/liferay-portal/pull/1379/files#diff-10174dbf5a248993084321a6662596ff7faa394aed5696d0fd001030e3544a0bR140 ?

Yeah, it'll get merged with whatever global declaration of the interface is there. The thing is you need to make sure everyone contributing those types is loaded all the time which is not the case currently... need to figure out a different approach, likely involving @types and typeRoots or sth like that to avoid the explicit imports... 🤔

Of course, even better would be if we stopped using the Liferay global object 😂

Of course, of course,... 😅

izaera · 2021-08-20T10:39:27Z

...frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/add_params.ts

 */
-export default function addParams(params, baseUrl) {
+export default function addParams(params: string | object, baseUrl: string) {


{ [key:string]: string }... does that have a universal name ?

Nope, but we can create one if it is very common...

I usually create types for this kind of option arguments. So something like this I think it's possible:

addParamsOptions { knownOption1: string; knownOption2: boolean; [key:string]: string; }

If there are no known options just the [key:string]: string; part. But having it as a named (and exported) type is usually handy when you need to pass those options through several function calls and have them typed.

Of course I'm not saying 100% of the time you must do this, but if it is a public API there are many benefits in having a named type for this, IMO.

izaera · 2021-08-20T10:43:07Z

.../frontend-js-web/src/main/resources/META-INF/resources/liferay/util/get_portlet_namespace.ts

 */
-export default function getPortletNamespace(portletId) {
+export default function getPortletNamespace(portletId: string) {


runtime type guards, keep'em or drop'em?

I would say you still need them in case someone calls you from plain JavaScript...

However, if we decide to do runtime type checks, we should create a small utility for that, so that code is cleaner and less error prone. Something like:

Ensure.isString(portletId);

That we put at the top of methods.

izaera · 2021-08-20T10:47:04Z

...s/frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/navigate.ts

+	url: string | URL,
+	listeners?: {[key: string]: Callback}
+) {
+	url = url instanceof URL ? String(url) : url;


I have a similar in FilePath I think...

I use the AnyPath type for string | FilePath and FilePath.coerce to do the cast.

izaera · 2021-08-20T10:49:28Z

modules/apps/frontend-js/frontend-js-web/test/liferay/util/portlet_url/create_action_url.ts


 describe('Liferay.Util.PortletURL.createActionURL', () => {
 	it('returns a URL object with a href parameter containing the p_p_lifecycle parameter set to 1', () => {
 		Liferay = {
 			ThemeDisplay: {
 				getPortalURL: jest.fn(() => 'http://localhost:8080'),
 			},
-		};
+		} as any;


Yes. But that's expected. That's why you have to use libraries like mockito in Java, too 🤷

markocikos · 2021-08-20T11:08:50Z

modules/apps/frontend-js/frontend-js-web/src/main/resources/META-INF/resources/index.ts

+
+// @ts-ignore
+


Is there a way to @ts-ignore the whole file?

Yes, you can do // @ts-nocheck, but then I wouldn't be able to slowly clean them up 😉

markocikos · 2021-08-20T11:19:24Z

modules/apps/frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/types.ts

+export interface ILiferay {
+	after(eventName: string, callback: Callback): void;
+	before(eventName: string, callback: Callback): void;
+	on(eventName: string, callback: Callback): void;
+	once(eventName: string, callback: Callback): void;
+	onceAfter(eventName: string, callback: Callback): void;
+}
+
+// Properties from `frontend-js-spa-web/init`
+
+export interface ILiferay {


Will the second export interface ILiferay override the first?

No, they merge as per Declaration Merging which is what I thought we could leverage to compose them from different modules.

In here, I just went with it to clearly outline the different origin of the properties, but that should be moved to its own module at some point. Or not 🤷

Interesting! Looks powerful, but also quite quirky, with a lot of edge cases.

bryceosterhaus · 2021-08-20T16:03:26Z

did we ever codify some guidelines or best practices in Clay or the other repos that we used to test TS with? Any hints to share?

Not sure what you mean by testing TS with. We do have a short TS Guidlines doc and especially with react we recommend react-typescript-cheatsheet. Honestly though, I think the most helpful thing is to check in other places how TS is used, it is one of those tools that can be really overdone and you spend 95% of your time creating complex type structures. Generally I like to aim for simple and concise. It is going to take an external use a significant amount of time just to understand the type, I think we should go simpler.

I'll check out the code here on Monday, I won't have time to give it a deep dive yet.

jbalsas · 2021-08-20T19:47:08Z

Hey @bryceosterhaus!

Not sure what you mean by testing TS with

Pardon my english 😂 . By testing I meant "trying it out". We rolled it out in some of those projects so that we could get some hands-on experience and some initial view on possible issues and mitigations to help others get on board easily. So, my question is... what were those learnings? Which things went well, what helped, what didn't...?

Hope that makes it clear-er :)

bryceosterhaus

Overall, this looks pretty good to me! It doesn't look nearly as bad as I might have originally thought it would.

So, my question is... what were those learnings? Which things went well, what helped, what didn't...?

I don't think there were any learnings that were concrete or "one-size-fits-all" enough to document.

Personally, at a high-level view I have made a few observations about TS

Migrating existing code to TS is usually harder than I originally think
Writing TS code as a whole is typically easier than I think
The more TS you have, the easier it is to write and migrate TS.
Copy TS patterns from existing code (both your own and 3rd party)
Always aim for simple types, type gymnastics typically indicate a code smell

bryceosterhaus · 2021-08-23T19:34:16Z

modules/apps/frontend-js/frontend-js-state-web/src/main/resources/META-INF/resources/State.ts

@@ -16,6 +16,7 @@ import SubscriberMap from './SubscriberMap';
 import deepFreeze from './deepFreeze';

 import type {Immutable} from './types';
+import * as Types from 'frontend-js-web/src/main/resources/META-INF/resources/liferay/types';


I actually prefer having to import types explicitly instead of them being globally available everywhere. I think this would also make sense for java devs to see types imported as well.

Plus, you can explicitly import types with ts import type {Liferay} from '/some-path/here';

bryceosterhaus · 2021-08-23T19:37:45Z

...frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/add_params.ts

 */
-export default function addParams(params, baseUrl) {
+export default function addParams(params: string | object, baseUrl: string) {


we can replace it with Record<string, string>
+1

I prefer we don't create a global [key:string]: string; helper, as I think it may lead people to abuse it when they don't want to define a proper interface.

bryceosterhaus · 2021-08-23T19:39:41Z

...tend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/build_fragment.ts

 	const div = document.createElement('div');

 	div.innerHTML = `<br>${htmlString}`;

-	div.removeChild(div.firstChild);
+	div.removeChild(div.firstChild!);


plaguing our codebase with non-null assertions doesn't feel right

I agree that it doesn't feel right to use these everywhere. If we do use it, I would prefer we have an explicit comment for why we are using it. So for this case, we can comment saying the <br> guarantees that there is a first node.

bryceosterhaus · 2021-08-23T19:40:49Z

.../frontend-js-web/src/main/resources/META-INF/resources/liferay/util/get_portlet_namespace.ts

 */
-export default function getPortletNamespace(portletId) {
+export default function getPortletNamespace(portletId: string) {


runtime type guards, keep'em or drop'em?

I vote for keeping them since, like @izaera mentioned, you can still run this from plain js.

bryceosterhaus · 2021-08-23T19:43:43Z

.../frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/html_util.ts

@@ -12,7 +12,7 @@
 * details.
 */

-const MAP_HTML_CHARS_ESCAPED = {
+const MAP_HTML_CHARS_ESCAPED: {[key: string]: string} = {


Instead of a generic object type, I think you can use as const at the end of the object declaration.

~~True that! Thanks!~~

Tried but didn't work since we later do:

export function escapeHTML(string: string) { return string.replace( HTML_ESCAPE, (match) => MAP_HTML_CHARS_ESCAPED[match] ); }

Which will give this lovely:

Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ readonly '"': """; readonly '&': "&"; readonly "'": "'"; readonly '/': "/"; readonly '<': "<"; readonly '>': ">"; readonly '`': "`"; }'.

No index signature with a parameter of type 'string' was found on type '{ readonly '"': """; readonly '&': "&"; readonly "'": "'"; readonly '/': "/"; readonly '<': "<"; readonly '>': ">"; readonly '`': "`"; }'

I think it would work if we did something like a switch statement or if-elses with MAP_HTML_CHARS_ESCAPED['>']...

Going to go with the Record<string, string> approach for now. Do you see a better way to tackle this?

Yeah I think I would just go with Record for now. I tried to mess around with the file a bit to get the types really locked in, but it required some gymnastics and ugly type declarations. Even though I was able to get the types accurate, I think the complexity of the declarations made it more complex than needed.

bryceosterhaus · 2021-08-23T19:43:53Z

.../frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/html_util.ts

@@ -24,7 +24,7 @@ const MAP_HTML_CHARS_ESCAPED = {

 export {MAP_HTML_CHARS_ESCAPED};

-const MAP_HTML_CHARS_UNESCAPED = {};
+const MAP_HTML_CHARS_UNESCAPED: {[key: string]: string} = {};


Similar to #1379 (comment) and on top of that, we also do:

Object.entries(MAP_HTML_CHARS_ESCAPED).forEach(([char, escapedChar]) => { MAP_HTML_CHARS_UNESCAPED[escapedChar] = char; });

Which also requires string-based key access. I get the feeling this could be reworked somehow if we wanted, but should be okay for now...

Yeah thats cool with me. Part of the issue is that TS makes the types a little more generic when you use methods on Object. For example, Object.keys returns Array<string>, instead of the explicit key names event when it is declared as const.

bryceosterhaus · 2021-08-23T19:47:10Z

.../frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/html_util.ts

 	return string.replace(HTML_UNESCAPE, (match) => {
 		return new DOMParser().parseFromString(match, 'text/html')
-			.documentElement.textContent;
+			.documentElement.textContent!;


Instead of asserting as non-null, I think it might be more explicit to do

Suggested change

.documentElement.textContent!;

.documentElement.textContent || '';

bryceosterhaus · 2021-08-23T19:51:29Z

modules/apps/frontend-js/frontend-js-web/test/liferay/util/add_params.ts

-		expect(() => addParams(1)).toThrow(TypeError);
-
-		expect(() => addParams(['one', 'two'])).toThrow(TypeError);
+		expect(() => addParams(1 as any, sampleUrl)).toThrow(TypeError);


I wonder if we can just remove these runtime tests because we are essentially already testing types via ts compiler. 🤷

bryceosterhaus · 2021-08-23T19:52:10Z

modules/apps/frontend-js/frontend-js-web/test/liferay/util/portlet_url/create_portlet_url.ts

+		(Liferay.ThemeDisplay.getPortalURL as jest.Mock<
+			string,
+			[]
+		>).mockRestore();


Maybe to avoid things like this for global objects we can create a base interface for Liferay with generic types to be able to use it in tests and another one for development.

jbalsas · 2021-08-25T12:51:14Z

Hey @izaera, @markocikos, @bryceosterhaus, @matuzalemsteles, I've pushed some additional conversions in case you want to take a look.

I removed the global Liferay ambient namespace approach to reduce the noise and so far I haven't been able to find a nice way for different modules to easily collaborate to a global namespace (which is a rather bad practice anyways). I haven't been able to reliable find a way get this done. So far, the reliable options I have are:

Put everything Liferay inside frontend-js-web since most other projects will depend on it and see the types
Use triple slash path directives. This works, but one needs to specify the paths to every piece of the declaration through several modules
Use type (or any other) type of imports to bring in the types. Same as with path directives, need to import as many as you want. Keeping in mind that if you only care about Liferay.SPA you'd only really need to import types from frontend-js-spa-web maybe makes this bearable. The only drawback is that you need to know where the global part of Liferay is coming from.
Create some type of intermediate project where we can do the aggregation of Liferay bits so we only need to import that one (and additionally maybe publish it so we can get it from @types)

From time to time, other things like project references or typeRoots seem to work, but then they stop working, so I'm not 100% sure yet what's going on.

I'd say, following @bryceosterhaus's advice that we could start small by putting everying inside the same liferay.d.ts in frontend-js-web and live with it and hopefully reduce the amount of global access we have overtime. We can also split it up and load the types on a per-usage need 🤷 Thoughts?

jbalsas · 2021-08-25T12:52:16Z

...js-web/src/main/resources/META-INF/resources/liferay/modal/components/SimpleInputModal.es.js

@@ -61,6 +61,7 @@ const SimpleInputModal = ({
 			document.querySelector(`#${namespace}form`)
 		);

+		// eslint-disable-next-line @liferay/portal/no-global-fetch


@bryceosterhaus, @kresimir-coko, this will need an adjustment to no-global-fetch since we explicitly look for fetch.es as a valid import which we won't have any longer... do you think we could work on this while this PR sits over here?

It's definitely something that we can investigate, I'll open an issue for it. Here it is liferay/liferay-frontend-projects#624

... which we won't have any longer.

@jbalsas care to expand on this statement? Are we going to be using both fetch.es and fetch or is there a PR that replaces usages of fetch.es to fetch. If I modify the rule to error when it sees fetch.es DXP will scream in pain.

Are we going to be using both fetch.es and fetch or is there a PR that replaces usages of fetch.es to fetch. If I modify the rule to error when it sees fetch.es DXP will scream in pain.

This PR 😉

jbalsas · 2021-08-25T12:53:15Z

modules/apps/frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/align.ts

+		? node.documentElement
+		: node;
+
+	return element[`client${property}` as ClientProp];


Brackets access is always tricky... 🤔

This will work in TS 4.4 since it accepts Template String Pattern Index Signatures after microsoft/TypeScript#44512!! 🎉

jbalsas · 2021-08-25T12:53:49Z

modules/apps/frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/align.ts


 	const transform =
-		style.mozTransform || style.msTransform || style.transform;
+		(style.mozTransform as string) || style.msTransform || style.transform;


Not sure what did I do here 🤔

I think it was just a leftover from before I typed it as VendorPrefixedCSSStyleDeclaration 😂

jbalsas · 2021-08-25T12:56:13Z

.../frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/util/is_object.ts

@@ -16,7 +16,7 @@
 * Returns true if the specified value is an object. This includes arrays
 * and functions.
 */
-export default function isObject(val: any) {
+export default function isObject<T>(val: T): val is NonNullable<T> {


I think this should infer never when called with other types. For example, const a = 'a'; if (isObject(a)) {...} will consider that a is of type string instead of never.

bryceosterhaus · 2021-08-25T20:02:17Z

putting everying inside the same liferay.d.ts in frontend-js-web and live with it and hopefully reduce the amount of global access we have overtime. We can also split it up and load the types on a per-usage need 🤷 Thoughts?

I like this approach. It's very simple to start with and it allows us to consolidate first and then determine the best next steps forward after.

matuzalemsteles · 2021-09-30T18:03:43Z

Oh I really like it too, it feels simpler and cleaner compared to having to import it, my 👍 to go with it 😁.

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

jbalsas · 2021-11-18T20:40:14Z

Well... I think it's obvious that I won't be around long enough to see this done... 😢

I just rebased and pushed the branch with what I had so far. I hit a bit of an egg and chicken type of problem with the current approach of a global.env.d.ts file since this makes some modules (like frontend-js-web) fail to compile because it finds sources outside rootDir. I tried several different approaches but wasn't really successful, so fixing this is now up to you! 💪

I'll keep this repo and branch around for some time, but it would be advisable for someone to keep track of it independently just in case I get trigger-happy with the "delete repo" button 😂

bryceosterhaus · 2021-11-18T20:51:29Z

Well... I think it's obvious that I won't be around long enough to see this done... 😢

We actually have been meaning to tell you, you aren't allowed to leave until this is done... Glad to have you around awhile! 😂

kresimir-coko · 2021-11-25T13:22:34Z

I pulled this, rebased and ran gradlew clean deploy from frontend-js-web, and here's the gist of all the errors it produced.

Most of the errors seem to be coming from frontend-js-spa-web and frontend-js-state-web, with a few of them coming from a couple files inside test/liferay/util/...

The commit containing changes triggered by formatSource and deploy can be found on kresimir-coko@b306dd8

@bryceosterhaus I guess next steps would be to get rid of these errors so that it can be actually deployed

julien · 2021-11-25T13:45:46Z

@bryceosterhaus I guess next steps would be to get rid of these errors so that it can be actually deployed

I guess you're right 👍

kresimir-coko · 2021-12-08T11:08:00Z

Closed in favor of #1732

liferay-continuous-integration added the ci:test:sf - pending label Aug 20, 2021

jbalsas commented Aug 20, 2021

View reviewed changes

liferay-continuous-integration added ci:test:relevant - pending ci:test:stable - pending labels Aug 20, 2021

jbalsas commented Aug 20, 2021

View reviewed changes

liferay-continuous-integration added ci:test:sf - failure and removed ci:test:sf - pending labels Aug 20, 2021

jbalsas commented Aug 20, 2021

View reviewed changes

liferay-continuous-integration added ci:test:relevant - failure and removed ci:test:relevant - pending labels Aug 20, 2021

izaera reviewed Aug 20, 2021

View reviewed changes

markocikos reviewed Aug 20, 2021

View reviewed changes

bryceosterhaus reviewed Aug 23, 2021

View reviewed changes

jbalsas commented Aug 25, 2021

View reviewed changes

kresimir-coko mentioned this pull request Aug 25, 2021

Adjust no-global-fetch rule to conform with TS migration patterns liferay/liferay-frontend-projects#624

Closed

4 tasks

jbalsas added 22 commits November 18, 2021 20:31

LPS-X Refactors logic to help TS infer the proper type for portletID

4b22739

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`.

LPS-X Replaces {[key: string]: string} with Record<string, string>

be8ced0

LPS-X Ports fetch utility to TS

fb25f07

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

LPS-X Migrates align utility to TS

6ea38eb

LPS-X Converts Address utilities to TS

5f872ac

LPS-X Provides a global env.d.ts file for ambient types

17e37e7

LPS-X Converts EventEmitter utilities to TS

23ce0ab

LPS-X Converts run_scripts_in_element utility to TS

b4136f9

LPS-X Modularizes Liferay global interface for better readability

be97819

LPS-X Converts module frontend-js-spa-web to TS

29a4640

LPS-X Converts toggle_disabled utility to TS

c2201e3

LPS-X Converts focus_form_field, in_browser_view, get_element and get…

bd30aa4

…_dom utilities to TS

LPS-X WIP convert module frontend-js-spa-web to TS

092dd31

kresimir-coko mentioned this pull request Dec 8, 2021

[RFC] TS migration patterns #1732

Closed

kresimir-coko closed this Dec 8, 2021

	.documentElement.textContent!;
	.documentElement.textContent \|\| '';

[RFC] TS migration patterns #1379

[RFC] TS migration patterns #1379

Conversation

jbalsas commented Aug 20, 2021

liferay-continuous-integration commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liferay-continuous-integration commented Aug 20, 2021

❌ ci:test:sf - 0 out of 1 jobs passed in 2 minutes

Base Branch:

Sender Branch:

1 Failed Jobs:

For more details click here.

liferay-continuous-integration commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

jbalsas Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

markocikos Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas commented Aug 20, 2021

liferay-continuous-integration commented Aug 20, 2021

❌ ci:test:relevant - 0 out of 1 jobs passed in 15 minutes

Base Branch:

Upstream Comparison:

1 Failed Jobs:

For more details click here.

Failures unique to this pull:

Job Results:

For upstream results, click here.

Test bundle downloads:

liferay-continuous-integration commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

jbalsas Aug 24, 2021 •

edited

Loading

jbalsas Aug 20, 2021 •

edited

Loading

jbalsas Aug 20, 2021 •

edited

Loading

markocikos Aug 20, 2021 •

edited

Loading

jbalsas Aug 24, 2021 •

edited

Loading

jbalsas Aug 20, 2021 •

edited

Loading

jbalsas Aug 20, 2021 •

edited

Loading