Fix/mapped generic type with date

[jackey8616]

All Submissions:

• [x] Have you followed the guidelines in our Contributing document?
• [x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• [x] Have you written unit tests?
• [x] Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
• [x] This PR is associated with an existing issue?

Closing issues Closes #1596

Potential Problems With The Approach I'm not sure this fix will effect other not covered cases. Need folks to help me figure it out are there any possible missed cases.

[jackey8616]

@WoH need a rerun. thx

[WoH]

Is this needed if the declaration is included? Usually we handle these high up in the type resolver (see: 'Promise').

I am generally very sceptical about these since they basically opt out of everything predictable.

[jackey8616]

Is this needed if the declaration is included? Usually we handle these high up in the type resolver (see: 'Promise').

I am generally very sceptical about these since they basically opt out of everything predictable.

I'm not sure the meaning of declaration is included, could you give me more detail about it?

The error is cause by generic type name cannot determine if given T is Date.
Type resolve itself is working good. Once the calcRefTypeName could name it, everything works.

[WoH]

I am worried about any <T extends Date> where Date is not the one we now assume it is.

[WoH]

I briefly debugged this. We have declarations for Date, 6 of them actually. The fix should be in how we get the right one, not papering over a fix with another fix.

[jackey8616]

I briefly debugged this. We have declarations for Date, 6 of them actually. The fix should be in how we get the right one, not papering over a fix with another fix.

I do noticed that there is 6 declarations (5 interface, 1 value). The error is cause by getModelTypeDeclarations omitted all typescript native library which Date is also included it.

Here comes 2 solutions:

1. escape like this:

     modelTypes = modelTypes.filter(modelType => {
       // remove types that are from typescript e.g. 'Account'
       switch (typeName) {
         case 'Date':
           return modelType;
         default: {
           const srcFileName = modelType.getSourceFile().fileName;
           return srcFileName.replace(/\\/g, '/').toLowerCase().indexOf('node_modules/typescript') <= -1;
         }
       }
     });

2. remove the filter I checked the definition of Date, found out that inside es2015, it seems nothing critical. Removing the filter is ok for all test cases.

Do you still remember anything about this native escape?

[WoH]

I briefly debugged this. We have declarations for Date, 6 of them actually. The fix should be in how we get the right one, not papering over a fix with another fix.

I do noticed that there is 6 declarations (5 interface, 1 value). The error is cause by getModelTypeDeclarations omitted all typescript native library which Date is also included it.

Here comes 2 solutions:

1. escape like this:

     modelTypes = modelTypes.filter(modelType => {
       // remove types that are from typescript e.g. 'Account'
       switch (typeName) {
         case 'Date':
           return modelType;
         default: {
           const srcFileName = modelType.getSourceFile().fileName;
           return srcFileName.replace(/\\/g, '/').toLowerCase().indexOf('node_modules/typescript') <= -1;
         }
       }
     });

2. remove the filter I checked the definition of Date, found out that inside es2015, it seems nothing critical. Removing the filter is ok for all test cases.

Do you still remember anything about this native escape?

Iirc there's a lot of bs in there. Basically all environments, as well as type declarations TS uses. Maybe we can decide based on the lib that is used from tsconfig. Otherwise we'd end up including a lot.

[jackey8616]

Iirc there's a lot of bs in there. Basically all environments, as well as type declarations TS uses. Maybe we can decide based on the lib that is used from tsconfig. Otherwise we'd end up including a lot.

I'm not sure should we keep this line or not. I noticed that this line is exists at very beginning and add by Luke, and I've no idea why he wants to escape some type called Account like he commented with // remove types that are from typescript e.g. 'Account'. Also, there is no type named Account in es5 ~ es2023.

Shall we try remove this?

[github-actions[bot]]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

[veiper93]

Is there any indication of whether and when this pull request will be merged?

[jackey8616]

@WoH Any news?

[WoH]

I have no clue, but removing and bumping a major version seems reasonable

[github-actions[bot]]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

[veiper93]

Will this be reopened and hopefully merged, or is it closed for good?