-
Notifications
You must be signed in to change notification settings - Fork 1
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
[RFC] TS migration patterns #1379
Changes from 1 commit
0fd4a33
c603114
70b78e0
7a98e2d
0d69c50
2c1f090
2c3f8f8
4b22739
be8ced0
fb25f07
6ea38eb
eda40b9
0a7714e
5f872ac
17e37e7
23ce0ab
b4136f9
be97819
29a4640
c2201e3
bd30aa4
092dd31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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.
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,17 +12,15 @@ | |
* details. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
import createActionURL from '../../../../src/main/resources/META-INF/resources/liferay/util/portlet_url/create_action_url.es'; | ||
import createActionURL from '../../../../src/main/resources/META-INF/resources/liferay/util/portlet_url/create_action_url'; | ||
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mocking can get tricky... 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. But that's expected. That's why you have to use libraries like mockito in Java, too 🤷 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe using |
||
|
||
const portletURL = createActionURL( | ||
'http://localhost:8080/group/control_panel/manage?p_p_id=foo' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,34 +12,35 @@ | |
* details. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
import createPortletURL from '../../../../src/main/resources/META-INF/resources/liferay/util/portlet_url/create_portlet_url.es'; | ||
import createPortletURL from '../../../../src/main/resources/META-INF/resources/liferay/util/portlet_url/create_portlet_url'; | ||
|
||
describe('Liferay.Util.PortletURL.createPortletURL', () => { | ||
afterEach(() => { | ||
Liferay.ThemeDisplay.getPortalURL.mockRestore(); | ||
(Liferay.ThemeDisplay.getPortalURL as jest.Mock< | ||
string, | ||
[] | ||
>).mockRestore(); | ||
Comment on lines
+19
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😬 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}); | ||
|
||
beforeEach(() => { | ||
Liferay = { | ||
ThemeDisplay: { | ||
getPortalURL: jest.fn(() => 'http://localhost:8080'), | ||
}, | ||
}; | ||
} as any; | ||
}); | ||
|
||
it('throws an error if basePortletURL is not a string', () => { | ||
expect(() => createPortletURL({portlet: 'url'}, {foo: 'bar'})).toThrow( | ||
'basePortletURL parameter must be a string' | ||
); | ||
expect(() => | ||
createPortletURL({portlet: 'url'} as any, {foo: 'bar'}) | ||
).toThrow('basePortletURL parameter must be a string'); | ||
}); | ||
|
||
it('throws an error if parameters is not an object', () => { | ||
expect(() => | ||
createPortletURL( | ||
'http://localhost:8080/group/control_panel/manage', | ||
'foo:bar' | ||
'foo:bar' as any | ||
) | ||
).toThrow('parameters argument must be an object'); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/** | ||
* Copyright (c) 2000-present Liferay, Inc. All rights reserved. | ||
* | ||
* This library is free software; you can redistribute it and/or modify it under | ||
* the terms of the GNU Lesser General Public License as published by the Free | ||
* Software Foundation; either version 2.1 of the License, or (at your option) | ||
* any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more | ||
* details. | ||
*/ | ||
/** | ||
* Returns an action portlet URL in form of a URL object by setting the lifecycle parameter | ||
*/ | ||
export default function createActionURL(basePortletURL: string, parameters?: { | ||
[key: string]: string; | ||
}): URL; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/** | ||
* Copyright (c) 2000-present Liferay, Inc. All rights reserved. | ||
* | ||
* This library is free software; you can redistribute it and/or modify it under | ||
* the terms of the GNU Lesser General Public License as published by the Free | ||
* Software Foundation; either version 2.1 of the License, or (at your option) | ||
* any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more | ||
* details. | ||
*/ | ||
/** | ||
* Returns a portlet URL in form of a URL Object | ||
*/ | ||
export default function createPortletURL(basePortletURL: string, parameters?: { | ||
[key: string]: string; | ||
}): URL; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/** | ||
* Copyright (c) 2000-present Liferay, Inc. All rights reserved. | ||
* | ||
* This library is free software; you can redistribute it and/or modify it under | ||
* the terms of the GNU Lesser General Public License as published by the Free | ||
* Software Foundation; either version 2.1 of the License, or (at your option) | ||
* any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more | ||
* details. | ||
*/ | ||
/** | ||
* Returns a render portlet URL in form of a URL object by setting the lifecycle parameter | ||
*/ | ||
export default function createRenderURL(basePortletURL: string, parameters?: { | ||
[key: string]: string; | ||
}): URL; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/** | ||
* Copyright (c) 2000-present Liferay, Inc. All rights reserved. | ||
* | ||
* This library is free software; you can redistribute it and/or modify it under | ||
* the terms of the GNU Lesser General Public License as published by the Free | ||
* Software Foundation; either version 2.1 of the License, or (at your option) | ||
* any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more | ||
* details. | ||
*/ | ||
/** | ||
* Returns a resource portlet URL in form of a URL object by setting the lifecycle parameter | ||
*/ | ||
export default function createResourceURL(basePortletURL: string, parameters?: { | ||
[key: string]: string; | ||
}): URL; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/** | ||
* Copyright (c) 2000-present Liferay, Inc. All rights reserved. | ||
* | ||
* This library is free software; you can redistribute it and/or modify it under | ||
* the terms of the GNU Lesser General Public License as published by the Free | ||
* Software Foundation; either version 2.1 of the License, or (at your option) | ||
* any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more | ||
* details. | ||
*/ | ||
export {}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/** | ||
* Copyright (c) 2000-present Liferay, Inc. All rights reserved. | ||
* | ||
* This library is free software; you can redistribute it and/or modify it under | ||
* the terms of the GNU Lesser General Public License as published by the Free | ||
* Software Foundation; either version 2.1 of the License, or (at your option) | ||
* any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more | ||
* details. | ||
*/ | ||
export {}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/** | ||
* Copyright (c) 2000-present Liferay, Inc. All rights reserved. | ||
* | ||
* This library is free software; you can redistribute it and/or modify it under | ||
* the terms of the GNU Lesser General Public License as published by the Free | ||
* Software Foundation; either version 2.1 of the License, or (at your option) | ||
* any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more | ||
* details. | ||
*/ | ||
export {}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/** | ||
* Copyright (c) 2000-present Liferay, Inc. All rights reserved. | ||
* | ||
* This library is free software; you can redistribute it and/or modify it under | ||
* the terms of the GNU Lesser General Public License as published by the Free | ||
* Software Foundation; either version 2.1 of the License, or (at your option) | ||
* any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more | ||
* details. | ||
*/ | ||
export {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure
portletID
can benull
here. That's a possible bug somewhere that we haven't yet seen. Thoughts on what should we really do here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few lines above:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You see, the thing is that if parameters aren't provided, then
portletID
can be null, but thengetPortletNamespace
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 casenull
is a valid thing to pass togetPortletNamespace
which doesn't seem to be the case since it will throw aTypeError
in that case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If parameters are not provided, we never reach
getPortletNamespace
:In this case, the value of
portletID
, that we maybe took frombasePortletURL
, 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 thebasePortletURL
will be used as intended, a base URL with no portlet-specific params.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 benull
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe putting everything inside one
if
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that should do it!