Skip to content
This repository has been archived by the owner on Feb 17, 2024. It is now read-only.

Lf/message server graceful error #267

Merged
merged 4 commits into from
Nov 9, 2020

Conversation

LisandroFernandezSF
Copy link
Contributor

What does this PR do?

It shows a new error message for LMS

What issues does this PR fix or reference?

@W-7586088@

@LisandroFernandezSF LisandroFernandezSF requested a review from a team as a code owner November 6, 2020 19:38
@codecov
Copy link

codecov bot commented Nov 6, 2020

The author of this PR, LisandroFernandezSF, is not an activated member of this organization on Codecov.
Please activate this user on Codecov to display this PR comment.
Coverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.
Please don't hesitate to email us at success@codecov.io with any questions.

sfsholden
sfsholden previously approved these changes Nov 6, 2020
@@ -21,7 +21,7 @@ export function getMacAddress() {
const macAddresses = Object.keys(networkInterfaces)
.reduce((macAddresses: string[], networkInterfaceName) => {
const networkInterfaceInfo =
networkInterfaces[networkInterfaceName];
networkInterfaces[networkInterfaceName] || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't the issue with network interfaces happening in this PR? I wasn't under the impression this current PR was experiencing issues with the tests. I'd be more curious to see if these changes have an effect on the other PR.

As a note, if we move forward with this change then tests will need to be added to machineId.test.ts. I'm hesitant to move forward with this change until we understand why your test scenario was failing though. There could be a different underlying issue that we need to address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed the issue with network interfaces is happening in both PRs.
Ok, I can dedicate more time to understand why the test scenario is failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you show me the test failure where this was happening on this PR too? Also, is it reproducible locally for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is not failing, the yarn build process is failing. It shows Object is possibly 'undefined' for this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be related to this issue nodejs/node#30504

Copy link
Contributor

Choose a reason for hiding this comment

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

If the issue is at build time I think the current change might be enough. If it is happening at runtime then we'll just return undefined when running getMacAddress(). Is there anything else I might be missing @sfsholden @LisandroFernandezSF ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, if this is happening at build time then I'm less concerned about adding in this change.

@@ -0,0 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`LWCMessageService rollup plugin load should throw error for @salesforce/messageChannel imports 1`] = `"You can't preview a component using Lightning Message Service. Test the component directly in the org"`;
Copy link

Choose a reason for hiding this comment

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

@LisandroFernandezSF - the message looks good. Do you need me to look at anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No Emily, I think that was everything


const service = new LWCMessageService();
const plugin = service.getPlugin();
function loadApexContinuation() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this has nothing to do with apex continuations so let's change the name of this function

Copy link
Contributor

@lcampos lcampos left a comment

Choose a reason for hiding this comment

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

looks good, let's see what Shelby thinks on that thread for the mac address code change

@LisandroFernandezSF LisandroFernandezSF merged commit 3bf38e5 into develop Nov 9, 2020
@LisandroFernandezSF LisandroFernandezSF deleted the lf/messageServerGracefulError branch November 9, 2020 22:42
sfsholden pushed a commit that referenced this pull request Nov 16, 2020
* fix: lms new message

* fix: using a real regexp

* fix: adding test file

* fix: tsc and lint errors
sfsholden pushed a commit that referenced this pull request Nov 16, 2020
* fix: lms new message

* fix: using a real regexp

* fix: adding test file

* fix: tsc and lint errors
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants