Fix unresolvable Vue typings #203
Conversation
WalkthroughThe pull request introduces significant structural changes to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
apps/vue/src/config.ts (1)
3-6: LGTM: Improved type safety forworkbook.The explicit typing of
workbookusingPick<Flatfile.CreateWorkbookConfig, ...>is an excellent improvement. It enhances type safety and makes the code more maintainable by clearly defining the expected shape of theworkbookobject.Consider using a type alias for better readability:
type WorkbookConfig = Pick<Flatfile.CreateWorkbookConfig, 'name' | 'labels' | 'sheets' | 'actions'>; export const workbook: WorkbookConfig = { // ... existing object ... };This would make the code more readable and reusable if you need to use this type elsewhere.
apps/vue/src/App.vue (3)
15-16: Assist with TypeScript type incompatibility of 'listener' and 'workbook' importsI notice that you're experiencing TypeScript type incompatibility issues with
listenerandworkloadtypes inApp.vue. This might be due to mismatched type definitions between the imported modules and the expected types inspaceProps.Would you like me to help identify and resolve the type incompatibilities?
20-20: Placeholder 'your_key' used for 'FLATFILE_PUBLISHABLE_KEY'The
FLATFILE_PUBLISHABLE_KEYis set to'your_key', which appears to be a placeholder. Ensure to replace this with your actual Flatfile publishable key before deploying.
52-54: Add return type annotation to 'toggleSpace' functionFor better type safety and clarity, consider adding a return type annotation to the
toggleSpacefunction.Apply this diff to add the return type annotation:
- const toggleSpace = () => { + const toggleSpace = (): void => {apps/vue/src/Simplified.vue (1)
49-49: Correct the typo in the 'name' property.The
nameproperty inspacePropsis set to'Trste!'. If this is a typo, consider correcting it to'Test!'or the intended name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
apps/vue/package.jsonis excluded by!**/*.jsonpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackages/vue/package.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
- apps/vue/src/App.vue (1 hunks)
- apps/vue/src/Simplified.vue (2 hunks)
- apps/vue/src/config.ts (1 hunks)
🔇 Additional comments (5)
apps/vue/src/config.ts (1)
1-1: LGTM: Type import added correctly.The addition of the type import from "@flatfile/api" is appropriate and necessary for the updated type definition of
workbook. Using a type-only import is a good practice as it doesn't affect the runtime bundle size.apps/vue/src/App.vue (1)
36-36: Verify the 'themeConfig' structurePlease verify that the
themeConfigstructure matches the expected format required by the Flatfile SDK. Incorrect nesting underrootmay prevent theme settings from being applied.If the SDK expects
primaryColorandtextColordirectly underthemeConfig, consider updating the code:- themeConfig: { root: { primaryColor: '#546a76', textColor: '#fff' }}, + themeConfig: { primaryColor: '#546a76', textColor: '#fff' },apps/vue/src/Simplified.vue (3)
70-70: Confirm the structure of 'themeConfig' matches Flatfile's expectations.The
themeConfignestsprimaryColorandtextColorunder arootkey. Verify that this structure aligns with Flatfile's theming configuration. If therootkey is unnecessary or if the keys should be at the top level, adjust the structure accordingly:-themeConfig: { root: { primaryColor: "#546a76", textColor: "#fff" } }, +themeConfig: { primaryColor: "#546a76", textColor: "#fff" },
85-85: Ensure proper destructuring when initializing Flatfile.Confirm that
initializeFlatfilereturns theSpacecomponent andOpenEmbedfunction as expected. If there are issues, verify the import and the API usage:const { Space, OpenEmbed } = initializeFlatfile(spaceProps);
66-69:⚠️ Potential issueVerify the 'operation' value in 'closeSpace' configuration.
The
operationis set to'submitActionFg'in thecloseSpaceobject. Ensure that'submitActionFg'is a valid operation recognized by the Flatfile API. If not, update it to the correct operation value as per the documentation.
| const publishableKey = FLATFILE_PUBLISHABLE_KEY | ||
| const spaceProps = ref({ | ||
| // apiUrl: 'https://platform.flatfile.com/api', | ||
| name: 'Trste!', |
There was a problem hiding this comment.
Typo in 'name' property of 'spaceProps'
The name property is set to 'Trste!', which might be a typo. Consider correcting it to 'Test!' or your intended space name.
Apply this diff to fix the typo:
- name: 'Trste!',
+ name: 'Test!',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: 'Trste!', | |
| name: 'Test!', |
| <button @click="toggleSpace()">{{ showSpace ? 'Close' : 'Open' }} space</button> | ||
| </div> | ||
|
|
||
| <div class="space-wrapper" v-if="showSpace"> | ||
| <Space /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the toggle logic for 'showSpace' and usage of 'OpenEmbed()'.
There appears to be a potential conflict between toggling showSpace to conditionally render <Space /> in the template and calling OpenEmbed() in the toggleSpace function. If OpenEmbed() handles displaying the Flatfile Space modal independently, managing showSpace might be redundant or cause inconsistent UI states. Consider the following:
- Option 1: Rely solely on
OpenEmbed()to open the Space modal and removeshowSpaceand the conditional<Space />rendering. - Option 2: Use
showSpaceto control the visibility of<Space />and remove theOpenEmbed()call if unnecessary.
Also applies to: 45-90
| onSubmit: async ({ job, sheet }: { job: any, sheet: any }) => { | ||
| const data = await sheet.allData() | ||
| console.log('onSubmit', data) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety by specifying exact types in 'onSubmit'.
The onSubmit function currently uses any types for job and sheet. To leverage TypeScript's benefits, consider importing and using the specific types provided by @flatfile/vue or defining appropriate interfaces:
import { FlatfileJob, FlatfileSheet } from '@flatfile/vue';
// ...
onSubmit: async ({ job, sheet }: { job: FlatfileJob; sheet: FlatfileSheet }) => {
// ...
},| onRecordHook: (record: any, event: any) => { | ||
| const firstName = record.get('firstName') | ||
| const lastName = record.get('lastName') | ||
| if (firstName && !lastName) { | ||
| record.set('lastName', 'Rock') | ||
| record.addInfo('lastName', 'Welcome to the Rock fam') | ||
| } | ||
| return record |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify precise types in 'onRecordHook' for better type safety.
In the onRecordHook function, record and event are typed as any. Define or import appropriate types to enhance type checking and reduce potential runtime errors:
import { FlatfileRecord, FlatfileEvent } from '@flatfile/vue';
// ...
onRecordHook: (record: FlatfileRecord, event: FlatfileEvent) => {
// ...
},| const toggleSpace = () => { | ||
| showSpace.value = !showSpace.value; | ||
| OpenEmbed() | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust 'toggleSpace' function based on the display logic chosen.
Depending on how you decide to manage the space display (as per the earlier suggestion), update the toggleSpace function accordingly:
- If using
OpenEmbed()exclusively, you might not need to toggleshowSpace.value. - If controlling the display with
showSpace, ensuretoggleSpaceonly updatesshowSpace.value.
| const spaceProps = ref({ | ||
| name: 'Trste!', | ||
| environmentId, | ||
| publishableKey, | ||
| sheet, | ||
| onSubmit: async ({ job, sheet }: { job: any, sheet: any }) => { | ||
| const data = await sheet.allData() | ||
| console.log('onSubmit', data) | ||
| }, | ||
| onRecordHook: (record: any, event: any) => { | ||
| const firstName = record.get('firstName') | ||
| const lastName = record.get('lastName') | ||
| if (firstName && !lastName) { | ||
| record.set('lastName', 'Rock') | ||
| record.addInfo('lastName', 'Welcome to the Rock fam') | ||
| } | ||
| return record | ||
| }, | ||
| render(props, ctx) { | ||
| const Space = props.Space | ||
|
|
||
| return ( | ||
| <div> | ||
| <div class="description"> | ||
| <button onClick={props.toggleSpace}>{ props.showSpace ? 'Close' : 'Open' } space</button> | ||
| </div> | ||
|
|
||
| {props.showSpace && <div class="space-wrapper"> | ||
| <Space /> | ||
| </div>} | ||
| </div> | ||
| ) | ||
| closeSpace: { | ||
| operation: 'submitActionFg', | ||
| onClose: () => { showSpace.value = false; }, | ||
| }, | ||
| themeConfig: { root: { primaryColor: "#546a76", textColor: "#fff" } }, | ||
| userInfo: { | ||
| name: 'my space name' | ||
| }, | ||
| spaceInfo: { | ||
| name: 'my space name' | ||
| }, | ||
| displayAsModal: true, | ||
| spaceBody: { | ||
| metadata: { | ||
| random: 'param' | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const { Space, OpenEmbed } = initializeFlatfile(spaceProps.value); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify 'spaceProps' by removing unnecessary 'ref'.
Since spaceProps does not appear to require reactivity, you can define it as a plain object. This eliminates the need to access .value when initializing Flatfile:
-const spaceProps = ref({
+const spaceProps = {
// ...properties
-});
+};Update the initialization accordingly:
-const { Space, OpenEmbed } = initializeFlatfile(spaceProps.value);
+const { Space, OpenEmbed } = initializeFlatfile(spaceProps);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const spaceProps = ref({ | |
| name: 'Trste!', | |
| environmentId, | |
| publishableKey, | |
| sheet, | |
| onSubmit: async ({ job, sheet }: { job: any, sheet: any }) => { | |
| const data = await sheet.allData() | |
| console.log('onSubmit', data) | |
| }, | |
| onRecordHook: (record: any, event: any) => { | |
| const firstName = record.get('firstName') | |
| const lastName = record.get('lastName') | |
| if (firstName && !lastName) { | |
| record.set('lastName', 'Rock') | |
| record.addInfo('lastName', 'Welcome to the Rock fam') | |
| } | |
| return record | |
| }, | |
| render(props, ctx) { | |
| const Space = props.Space | |
| return ( | |
| <div> | |
| <div class="description"> | |
| <button onClick={props.toggleSpace}>{ props.showSpace ? 'Close' : 'Open' } space</button> | |
| </div> | |
| {props.showSpace && <div class="space-wrapper"> | |
| <Space /> | |
| </div>} | |
| </div> | |
| ) | |
| closeSpace: { | |
| operation: 'submitActionFg', | |
| onClose: () => { showSpace.value = false; }, | |
| }, | |
| themeConfig: { root: { primaryColor: "#546a76", textColor: "#fff" } }, | |
| userInfo: { | |
| name: 'my space name' | |
| }, | |
| spaceInfo: { | |
| name: 'my space name' | |
| }, | |
| displayAsModal: true, | |
| spaceBody: { | |
| metadata: { | |
| random: 'param' | |
| } | |
| } | |
| }); | |
| const { Space, OpenEmbed } = initializeFlatfile(spaceProps.value); | |
| const spaceProps = { | |
| name: 'Trste!', | |
| environmentId, | |
| publishableKey, | |
| sheet, | |
| onSubmit: async ({ job, sheet }: { job: any, sheet: any }) => { | |
| const data = await sheet.allData() | |
| console.log('onSubmit', data) | |
| }, | |
| onRecordHook: (record: any, event: any) => { | |
| const firstName = record.get('firstName') | |
| const lastName = record.get('lastName') | |
| if (firstName && !lastName) { | |
| record.set('lastName', 'Rock') | |
| record.addInfo('lastName', 'Welcome to the Rock fam') | |
| } | |
| return record | |
| }, | |
| closeSpace: { | |
| operation: 'submitActionFg', | |
| onClose: () => { showSpace.value = false; }, | |
| }, | |
| themeConfig: { root: { primaryColor: "#546a76", textColor: "#fff" } }, | |
| userInfo: { | |
| name: 'my space name' | |
| }, | |
| spaceInfo: { | |
| name: 'my space name' | |
| }, | |
| displayAsModal: true, | |
| spaceBody: { | |
| metadata: { | |
| random: 'param' | |
| } | |
| } | |
| }; | |
| const { Space, OpenEmbed } = initializeFlatfile(spaceProps); |
Please explain how to summarize this PR for the Changelog:
Fixed unresolvable Vue TS typings
Tell code reviewer how and what to test:
This just fixes type resolution for the Vue package. I've also updated the Vue app to use script setup + typescript templates. This does cause a weird type incompatibility issue for listener + workload types within
App.vuethat I'm hoping someone from the flatfile team can look at.I do have another branch that modernizes the build tooling of the vue package itself if you're interested, that rollup config is pain.