-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
JS: Backport³ and more additions & fixes #3961
Conversation
Co-authored-by: oldip <[email protected]>
Co-authored-by: oldip <[email protected]>
Co-authored-by: xMasterX <[email protected]>
Co-authored-by: xMasterX <[email protected]>
Co-authored-by: Spooks4576 <[email protected]>
Co-authored-by: Spooks4576 <[email protected]>
Co-authored-by: Spooks4576 <[email protected]>
Co-authored-by: jamisonderek <[email protected]>
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.
Additional comments, in order of decreasing importance:
- I don't like how with the max length thing the fields have to be arranged in a specific way for the thing to work. Field ordering is absolutely not something that the ECMAScript specification guarantees. This is especially important with the new build pipeline underway.
- Not an issue with this particular PR, just a thing to note in regards to [FL-3918] Full-fledged JS SDK + npm packages #3963:
.d.ts
files would have to be moved, and the new declarations would have to get@version
doc comments. - I don't like that the file browser is not a view. But that's an issue for later, I don't think that this PR needs to change this.
LGTM otherwise
mJS does, properties are parsed from code top to bottom and stored in a linked list inserting at the head, so at the end of object parsing the properties are in exact inverse order from what one would expect. inversing the processing order of props in makeWith() also doesn't seem smart, would be so time inefficient due to it being a singly linked list from what i can see. but yes, not ideal to rely on this behavior, and also it is counterintuitive to the user. i see 2 possible solutions:
so really there is no solution. this is a chicken and egg problem. best compromise to me soudns liek resizing to atelast size specified by default data, and if user also provides a length that is incompatible with this, it is their issue to deal with order of processing
if the intention is to merge that pr before this one then ill make those changes as required |
We prefer to rely on specifications and not implementation details :) Like I said, gonna become very important with our new tools that are going to be processing JS code before mJS even sees it. With regards to the solution, I think we can just keep the buffer the maximum size out of all sizes that we know of (user-requested max length, user-provided default text, etc.) and explicitly throw an error if the requested max length is smaller than the length of the provided default text.
We'll see how it goes :) I don't know which one will get merged first, that depends on Aku. |
ah well, that sounds good enough. ill change it again to track default data length separately and error if length is specified less than that |
one thing im not sure of: string returned by mjs to c is not guaranteed to be null terminated. props pass just a const char* with no length value. this sounds like a problem waiting to happen to me. like will my strlen() for default text overflow? idk probably? lol |
@portasynthinca3 i dont have my flipper with me right now for another few hours, logic should be ok but i cannot test right now. only thing that remains is my concern with strings passed as props possibly not being nul terminated... will this be an issue? |
@Willy-JL This is a valid concern based on the doc comment. However, after reviewing the actual implementation of
Perhaps we're dealing with outdated documentation here. I personally have never gotten a string from mJS that isn't null terminated, and I'm convinced that my analysis shows that this can never happen. We might want to ask the mJS team about this, but their repo seems dead. P.S.: Holy cow, is mJS code ever hard to read ;_; |
Sounds great, thanks for the very detailed break down! |
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.
LGTM, assuming the functionality works as intended.
just tested on device and seems to work great! |
@skotopes fixed, works fine now (and i learned my lesson to actually flash what i code, not that but ported to another fork xD) also i mirrored the JS SDK changes porta made but for the additions of this PR, hope it looks fine |
;-) |
Good job everyone! |
What's new
Needs feedback:
Verification
Checklist (For Reviewer)