-
Notifications
You must be signed in to change notification settings - Fork 11
Typescript Migration #47
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi Andy, Wow, lot's of work here. Appreciate the effort! All comments below are "in my humble opinion." Overall, personally I'd just add jsdocs to the existing JS methods and call it a day. Ultimately typescript is about linting at design/build time, which proper docs also covers (at least in compatible editors or some external linter/builder). And/or generate/maintain a separate .dts type definitions file. I'm just not sure all the additional overhead of requiring TS is actually helpful (and I do have some practical experience with doing it both ways, in other projects). Code wise, this makes some changes which are not backwards compatible. Making some methods private, for example (eg. EDIT: Oh, and all the methods being renamed... why? Makes it definitely incompatible with all current implementations. It is difficult to compare the actual working code since a lot of the formatting and arrangement have changed. Some changes don't maintain current code style either (which could be changed of course but ideally in a separate commit/PR). Other unrelated changes are included, like formatting of changelog and readme. These are personal preferences, and including them here further complicates review. Cheers, |
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.
Todo's
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.
another todo
|
Hi @mpaperno Thank you for the feedback, and sorry for the late response getting back to you! I've made a start to address some of your feedback 😃,
The function renames were for readability more than anything 😅, but yeah sadly this does introduce some breaking changes with functions being renamed - and their parameters changing (either because they have reordered or more added), I'd be more than happy to write a migration guide or look at other ways to minimize this change (readd the functions, mark as deprecated and map it to the newer functions for example). EDIT 23/07 - I've been updating the description with a breakdown of the changes to each function to help with review - it only reflects the functions which are public Thank you very much again 😁 |
adds markdown files to prettierignore add a reuseable helper for validating arrays
fix prettier and eslint battling it out
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.
Todo
|
Marking as ready for review, all that is left is to add test's for each thing, but this PR is already bulky as is. I can either:
I'd also still like to add github actions for (either this pr or another):
|
Full migration of this project to typescript, here's a more detailed breakdown of these changes:
connect,checkForUpdate,disconnectandlog)eslint-config-airbnb-basedoes not support eslint 9 yet)require-from-app-root(is now done via passing as param)TriggerEvent(eventId: string, states?: Record<string, string>)function - https://www.touch-portal.com/api/index.php?section=communication_trigger_eventOn(TouchPortalIncomingEventType.Info)or if expecting a event from the clientOn(TouchPortalClientEvent.Update)npm link, no need to let the end user build the client)Renames the following functions:
choiceUpdate(id, value)updateChoice(id: string, value: string[])choiceUpdateSpecific(id, value, instanceId)updateSpecificChoice(id: string, instanceId: string, value: string[])connectorUpdate(id, value, data, isShortId = false)updateConnector(value: number, connectorId?: string, shortId?: string, data?: ConnectorData[])connectorUpdateMany(connectors)updateMultipleConnectors(connectors: { value: number; connectorId?: string; shortId?: string; data?: ConnectorData[]; }[])sendNotification(notificationId, title, msg, optionsArray)showNotification(notificationId: string, title: string, msg: string, options: NotificationOption[])settingUpdate(name, value)updateSetting(name: string, value: string)createStateMany(states)createMultipleStates(states: { id: string; desc: string; defaultValue: string, number , boolean; parentGroup?: string; forceUpdate?: boolean; }[])stateUpdate(id, value)updateState(id: string, value: string, number, boolean)stateUpdateMany(states)updateMultipleStates(states: { id: string; value: string, number, boolean }[])Changes the following function parameters
updateActionData(actionInstanceId, data)updateActionData(data: ActionData, instanceId?: string)choiceUpdateSpecific(id, value, instanceId)updateSpecificChoice(id: string, instanceId: string, value: string[])connectorUpdate(id, value, data, isShortId = false)updateConnector(value: number, connectorId?: string, shortId?: string, data?: ConnectorData[])shortIdshould be used or not, addedshortIdas a parameter so it can just be passed straight though - reading the docs however it is a mandatory parameter so more changes may be required https://www.touch-portal.com/api/index.php?section=communication_create_update_connector_data, changes will also be required to supportvalueDecimal(at the time of opening this pr I didn't really understand the description yet)checkForUpdate(githubUser, githubRepo, includePrerelease = false)checkForUpdate(githubUser: string, githubRepo: string, currentVersion: string, includePrerelease: boolean = false)repositoryMade the following functions private:
logIt)buildConnectorUpdate)Still todo: