[legacy-framework] Fix page generator with custom context#669
[legacy-framework] Fix page generator with custom context#669
Conversation
peaonunes
changed the title
| modelNames: this.options.modelNames, | ||
| ModelName: this.options.ModelName, | ||
| ModelNames: this.options.ModelNames, | ||
| importModelNames: this.getPathWithContext(), |
There was a problem hiding this comment.
Happy to change to a better name could not think about any other atm.
| } | ||
|
|
||
| getModelNameAndContext(modelName: string, context?: string): {model: string; context: string} { | ||
| getModelNameAndContext(modelName: string, context?: string): {model: string; context?: string} { |
There was a problem hiding this comment.
Found a bug here while testing the different input values.
Passing not setting the context was leading to contextSegments.= '' here. However, if we call path with empty string we gotta a wrong path:
console.log(path.join(''))
> .So I noticed a wrong it would generate the wrong import on the newly __importModelNames I've added. Since the options.context passed down to generators would be .. That potentially would affect other parts of the code as well.
|
Awesome, thank you so much!! :) I'll let @aem do a full review |
aem
left a comment
aem
left a comment
There was a problem hiding this comment.
wow, good find here! thanks for jumping on a fix. just a couple of small comments, overall looks great though!
| } | ||
| } | ||
|
|
||
| if (!!context && context !== '') { |
There was a problem hiding this comment.
super minor: can we use Boolean(context) instead of !!? it's just a little more explicit about the intent
There was a problem hiding this comment.
TS seems to think Boolean constructor is not equivalent to use !!. It's complaining.
So I changed to be != null and != undefined explicitly here 4f97355
| modelNames: this.options.modelNames, | ||
| ModelName: this.options.ModelName, | ||
| ModelNames: this.options.ModelNames, | ||
| importModelNames: this.getPathWithContext(), |
peaonunes
force-pushed
the
fix-replace-template-context
branch
from
e72f4ac to
783027e
Compare
| } | ||
| } | ||
|
|
||
| if (context !== undefined && context !== null && context !== '') { |
There was a problem hiding this comment.
This is good if you're happy with this solution! Two other options that are a little shorter:
if (context != null && context !== '')if (Boolean(context)) (context as string).split(/[\\/]/)
We can either take advantage of != null checking for both null and undefined but no other falsy values, or the fact that we're a little smarter than the TS compiler and just do a manual cast to string since we know the value can't be undefined at that point
There was a problem hiding this comment.
Good point! Happy to go with 2. Done here a8ca8fd
flybayer
changed the title
itsdillon
changed the title
3 participants
Closes: blitz-js/legacy-framework#910
What are the changes and their implications?
Fixes the page templates and the generations.
The paths on templates should be different from the
modelNamessince the generatedqueriesandmutationsmay have additional path ofcontextincluded.Added a new template value in the pages to refer to import path of the model considering the context.
Checklist