Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Upstream connection delays now properly handled in the TypeScript federates #1607

Merged
merged 13 commits into from
Mar 22, 2023

Conversation

byeonggiljun
Copy link
Collaborator

The previous version of this PR is lingua-franca/pull/1344

Relevant reactor-ts PR: reactor-ts/#138

This PR adds upstream connection delays in the generated TypeScript federate files.

@byeonggiljun byeonggiljun marked this pull request as ready for review February 28, 2023 03:03
@byeonggiljun
Copy link
Collaborator Author

It seems like there is a problem with handling after delays in docker. The test TypeScript/src/docker/federated/DistributedCountContainerized.lf fails. I'll turn this PR into a draft.

@byeonggiljun byeonggiljun marked this pull request as draft March 6, 2023 07:47
@petervdonovan
Copy link
Collaborator

petervdonovan commented Mar 9, 2023

Apparently the federates are not even being built because something in the generated code does not pass the type checks. Here is the snippet from the CI output that I think is most informative

    ERROR: never[]; }' is not assignable to type 'FederateConfig'. [c.ts:17:280]
    ERROR: never[]; }' is not assignable to type 'FederateConfig'. [c.ts:17:280]
    ERROR: TimeValue[][]; }' is not assignable to type 'FederateConfig'. [p.ts:17:294]
    ERROR: TimeValue[][]; }' is not assignable to type 'FederateConfig'. [p.ts:17:294]

@byeonggiljun
Copy link
Collaborator Author

That's weird... Because I cannot find any error on my local computer when I compile and run the test test/TypeScript/src/docker/federated/DistributedCountContainerized.
The CI is run with a master branch of the reactor-ts, right? If it is, there will be no type check error.

@petervdonovan
Copy link
Collaborator

petervdonovan commented Mar 9, 2023

I was able to reproduce the problem. In my local clone of reactor-ts (in my case, located at /home/peter/lf/reactor-ts), I checked out ts-common-timevalue, and in my local clone of lingua-franca, I checked out ts-upstream-delays. Then I ran the command

lfc --external-runtime-path /home/peter/lf/reactor-ts test/TypeScript/src/docker/federated/DistributedCountContainerized.lf 

and was able to reproduce the problem. Maybe double check to make sure that you passed the correct path to LFC using --external-runtime-path?

Edit: Actually, I think the issue is that --external-runtime-path is not working for me. I'm looking into it now.

petervdonovan added a commit that referenced this pull request Mar 9, 2023
When we make changes to reactor-ts and accompanying changes in
lingua-franca we cannot point to the version of reactor-ts that is
published on NPM. We need to clone a specific ref and point to it.

Locally, pointing to the correct runtime version fixes the problem that
I observed in #1607. If this change to the yaml file is correct then
this should make that PR mergeable.
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me, but it was marked as "draft" so it escaped my attention. Could you please add JavaDoc to the new function that you added in TSExtension? Thanks!

@lhstrh lhstrh marked this pull request as ready for review March 22, 2023 04:58
@lhstrh lhstrh changed the title Handling upstream connection delays in the TypeScript federates Upstream connection delays now properly handled in the TypeScript federates Mar 22, 2023
@lhstrh lhstrh added the bugfix label Mar 22, 2023
@lhstrh lhstrh added the typescript Related to TypeScript target label Mar 22, 2023
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that you had to change the package.json, but we can look into the reason behind that later. We can merge this now.

@lhstrh lhstrh merged commit 62ebfb7 into master Mar 22, 2023
@lhstrh lhstrh deleted the ts-upstream-delays branch March 22, 2023 19:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bugfix typescript Related to TypeScript target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants