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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/instrumentation/machineId.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.

macAddresses = macAddresses.concat(
networkInterfaceInfo.map(info => {
return info.mac;
Expand Down
1 change: 0 additions & 1 deletion src/server/LocalDevServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ export default class LocalDevServer {
}

config.addMiddleware(middleware);

const routes: ContainerAppExtension[] = [
projectMetadata(this.sessionNonce, this.project)
];
Expand Down
4 changes: 2 additions & 2 deletions src/server/__tests__/LocalDevServer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ jest.mock('@webruntime/server', () => {
super(...args);
mockServerConstructor(...args);
}
initialize() { }
start() { }
initialize() {}
start() {}
shutdown() {
this.emit('shutdown');
}
Expand Down
5 changes: 4 additions & 1 deletion src/server/config/WebruntimeConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { Plugin } from 'rollup';
import { ApexService, SchemaService } from '@communities-webruntime/services';
import { ResourceUrlService } from '../services/ResourceUrlService';
import { ApexContinuationService } from '../services/ApexContinuationService';
import { LWCMessageService } from '../services/LWCMessageService';

import alias from '@rollup/plugin-alias';

export default class WebruntimeConfig implements Config {
Expand Down Expand Up @@ -76,7 +78,8 @@ export default class WebruntimeConfig implements Config {
AppBootstrapService,
ImportMapService,
ResourceUrlService,
SchemaService
SchemaService,
LWCMessageService
];

this.bundle = [
Expand Down
41 changes: 41 additions & 0 deletions src/server/services/LWCMessageService.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { CompileService } from '@webruntime/api';
import path from 'path';

const APEX_LWC_MESSAGE_SERVICE_REGEX = /^(@salesforce\/messageChannel\/.*)/;

function matchesLWCMessageServiceScopedModule(id: string) {
return id && id.match(APEX_LWC_MESSAGE_SERVICE_REGEX);
}

/**
* Rollup plugin to resolve @salesforce/messageChannel
*
*/
function plugin() {
return {
name: 'rollup-plugin-lwc-message-service',

resolveId(id: string) {
return matchesLWCMessageServiceScopedModule(id) ? id : null;
},
load(id: any) {
const idParts = matchesLWCMessageServiceScopedModule(id);
if (idParts) {
throw new Error(
"You can't preview a component using Lightning Message Service. Test the component directly in the org"
);
}
return null;
}
};
}

export class LWCMessageService implements CompileService {
LisandroFernandezSF marked this conversation as resolved.
Show resolved Hide resolved
getPlugin() {
return plugin();
}

async initialize() {}

shutdown() {}
}
61 changes: 61 additions & 0 deletions src/server/services/__tests__/LWCMessageService.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { LWCMessageService } from '../LWCMessageService';
import { getLatestVersion } from '@webruntime/server/dist/commonjs/utils/utils';
import { PublicConfig } from '@webruntime/api';

jest.mock('@webruntime/server/dist/commonjs/utils/utils');

describe('LWCMessageService', () => {
let config: PublicConfig;

beforeEach(() => {
(getLatestVersion as jest.Mock).mockReturnValue('158104e2eb');
});

afterEach(() => {
jest.resetAllMocks();
});

it('returns a rollup plugin', () => {
const service = new LWCMessageService();
expect(service.getPlugin()).toBeTruthy();
});

describe('rollup plugin', () => {
describe('resolveId', () => {
it('should resolve paths with @salesforce/messageChannel', () => {
const service = new LWCMessageService();
const plugin = service.getPlugin();

const resolved = plugin.resolveId(
'@salesforce/messageChannel/SampleMessageChannel__c'
);
expect(resolved).toBe(
'@salesforce/messageChannel/SampleMessageChannel__c'
);
});
});

describe('load', () => {
it('should throw error for @salesforce/messageChannel imports', () => {
(getLatestVersion as jest.Mock).mockReturnValue('158104e2eb');

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

plugin.load(
'@salesforce/messageChannel/SampleMessageChannel__c'
);
}
expect(loadApexContinuation).toThrowErrorMatchingSnapshot();
});

it('should return null instead of throwing error for other than @salesforce/messageChannel imports', () => {
const service = new LWCMessageService();
const plugin = service.getPlugin();
const resolved = plugin.load('./lightningMessageService.html');

expect(resolved).toBeNull();
});
});
});
});
Original file line number Diff line number Diff line 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