-
Notifications
You must be signed in to change notification settings - Fork 400
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
fix(jest-transformer): update @salesforce/apex transform #924
Conversation
packages/@lwc/jest-transformer/src/test/modules/example/apex/__tests__/apex.test.js
Show resolved
Hide resolved
}); | ||
|
||
describe('example-apex', () => { | ||
it('default import is resolved Promise', () => { |
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.
This tests the default import eventually resolves, not that it's already resolved.
}) | ||
}); | ||
|
||
it('can import and call multiple default Apex imports', () => { |
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.
This tests that a second Apex import can be called (given the context of the first test above), not that multiple Apex imports can be called.
packages/@lwc/jest-transformer/src/test/modules/example/apex/apex.js
Outdated
Show resolved
Hide resolved
|
||
test('throws error if renamed default imports', ` |
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.
Why don't you want this or the test below?
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.
I removed them because we allow named imports for @salesforce/apex
now. I guess we could add special logic to see if the import is renaming the default, but I'm not sure how valuable that would be.
* shared. | ||
*/ | ||
const resolvedPromiseTemplate = babelTemplate(` | ||
global.MOCK_NAME = global.MOCK_NAME || function() { return Promise.resolve(); }; |
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.
Why not do this global assignment inside the catch on line 24 since it's only used in that code path? That avoids polluting the global var and evaluation when it's not required.
…pex.js Co-Authored-By: trevor-bliss <[email protected]>
Co-Authored-By: trevor-bliss <[email protected]>
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 minor issues
packages/@lwc/jest-transformer/src/test/modules/example/apex/__tests__/apexWithJestMock.test.js
Outdated
Show resolved
Hide resolved
Benchmark resultsBase commit: |
Details
The
@salesforce/apex
module import is different from the other salesforce scoped module imports for two reasons:import { refreshApex, getSObjectValue } from '@salesforce/apex';
@wire
adapter id.Changes in the PR are to support both cases. Additionally, there is some minor cleanup to the existing transforms logic.
Does this PR introduce a breaking change?