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, |
|
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
|
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: