build(all): use es2019 target to support on Node12 #925
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ijemmy
commented
May 31, 2022
Comment on lines
+18
to
+25
| const createCaptureAsyncFuncMock = function(provider: ProviderServiceInterface): CaptureAsyncFuncMock { | ||
| return jest.spyOn(provider, 'captureAsyncFunc') | ||
| .mockImplementation((methodName, callBackFn) => { | ||
| const subsegment = new Subsegment(`### ${methodName}`); | ||
| jest.spyOn(subsegment, 'flush').mockImplementation(() => null); | ||
| callBackFn(subsegment); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Note to @dreamorosi
I found that we missed the case that the subsegment IS defined in our test. So I add this to get coverage back to 100%
There was a problem hiding this comment.
Thanks for the note and good spot.
ijemmy
commented
May 31, 2022
Comment on lines
+10
to
20
| strategy: | ||
| matrix: | ||
| version: [12, 14, 16] | ||
| fail-fast: false | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - name: "Use NodeJS 16" | ||
| - name: "Use NodeJS" | ||
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: '16' | ||
| node-version: ${{ matrix.version }} | ||
| - name: Install [email protected] |
There was a problem hiding this comment.
Some unit tests fail in NodeJS12, so I make use of matrix strategy here to make sure that we don't introduce any issue later.
dreamorosi
approved these changes
May 31, 2022
|
node 12 tests failing :/ |
@flochaz It should fail until we release a new version that supports node12. |
Ah yeah true :) |
8 tasks
dreamorosi
pushed a commit
that referenced
this pull request
Aug 2, 2022
Switching to target 2019 to support Node12 Previously, we have been building our packages on target es2020. When customers consume our modules, it will still have ?.. If customers use NodeJS12 (which is es2019), it will throw an error SyntaxError: Unexpected token '.'. The . is the second character of ?. syntax. To fix this, we change all of the target to 2019. However, this cause a lot of breaks in our current code. 1. Branch coverage will drop as ?. is broken into a conditional statement. We need to add new test cases or tell TS that the value does exist by adding !. This is really case by case as the value cannot really be undefined in some case. 2. NodeJS12 doesn't have crypto.randomUUID that we use in e2e tests. So we have to add uuid as a devDependency This commit also adds the additional checks on NodeJS12 and NodeJS14. (We tested only on Node16). The failing one is Node12, and it fails on examples/sam package. This package uses the npm published version of our library. And it really should fail as the currently published library is target 2020. Once we publish a new version, this should be fixed automatically.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of your changes
See issue #899 on the bug description.
In target
es2020, it support?.so the TS transpiler will not transform this syntax.For example:
will be:
Previously, we have been building our packages on target
es2020. When customers consume our modules, it will still have?.. If customers use NodeJS12 (which ises2019), it will throw an errorSyntaxError: Unexpected token '.'. The.is the second character of?.syntax.To fix this, we change all of the target to 2019. However, this cause a lot of breaks in our current code.
?.is broken into a conditional statement. We need to add new test cases or tell TS that the value does exist by adding!. This is really case by case as the value cannot really beundefinedin some case.crypto.randomUUIDthat we use in e2e tests. So we have to adduuidas adevDependencyImportant note Notice that one of the
pr-lint-and-testfails. This is intentional. I added the additional checks on NodeJS12 and NodeJS14. (We tested only on Node16). The failing one is Node12, and it fails onexamples/sampackage. This package uses the npm published version of our library. And it really should fail as the currently published library is target 2020. Once we publish a new version, this should be fixed automatically.How to verify this change
Switch to NodeJS 12 and run all tests
Related issues, RFCs
#899
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.