Skip to content

Conversation

@pprice
Copy link
Contributor

@pprice pprice commented Aug 25, 2019

  • [js] Missing class name for generated services and processors; this makes instrumentation of outbound calls (e.g. which service are we calling in to) difficult. Generating a class name for es6 enables x.constructor.name to get generated name.
  • [js] Fix case where services have a method with an argument named "params", generated code generates an identifier in the "send_XXX" body named "params" which conflicts with the argument. Ensure the identifier in the body is uniquely named.
  • [js] Ensure that derived services (extends XYZService) correctly call super() in their constructor.
  • [ts] Ensure that derived services (extends XYZSerice) correctly specify the base class Client and Processor name
  • [js] Ensure that derived service clients do not redefine private fields (output, pClass, _seqId, _reqs) and call super();
  • [ts] Ensure derived service clients and processors do not redefine private fields in declaration.

@wadeDra
Copy link

wadeDra commented Sep 4, 2019

I found this error : "Must call super constructor in derived class before accessing 'this' or returning from derived constructor" when I am using thrift es6.
It is bug ? How can I avoid that?

@pprice
Copy link
Contributor Author

pprice commented Sep 4, 2019

@wadeDra does your service derive from another service? E.g. service Foo extends Base if so; this PR would fix that particular issue. I'm still trying to figure out why the travis-ci build failed (can't reproduce locally) so I can get this landed.

FWIW the approach I took for my local build process, was to write codemods / replacements to fix up the output until I could get this landed.

@stale
Copy link

stale bot commented Nov 3, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 3, 2019
@emmenlau
Copy link
Member

emmenlau commented Nov 3, 2019

Dear @pprice , your PR seems valuable, could you make further progress with the travis checks? If not, do you want to try and split it into smaller logical chunks? This can help isolate what breaks travis and maybe move ahead with the other parts more quickly?

@stale
Copy link

stale bot commented Nov 3, 2019

This issue is no longer stale. Thank you for your contributions.

@stale stale bot removed the wontfix label Nov 3, 2019
@wadeDra
Copy link

wadeDra commented Nov 20, 2019

any progress?

@pprice
Copy link
Contributor Author

pprice commented Dec 12, 2019

@emmenlau Let me look at this over the next couple of days. The travis failure did seem unrelated, but I'm happy to break it up if we can get some traction on merging the changes in.

@dcelasun
Copy link
Member

Travis failures are indeed unrelated. @pprice can you rebase from master? Also some tests would be nice.

Copy link
Member

@Jens-G Jens-G left a comment

Choose a reason for hiding this comment

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

Travis failures are indeed unrelated. @pprice can you rebase from master? Also some tests would be nice.

@pprice
Copy link
Contributor Author

pprice commented Feb 1, 2020

Rebased; let's give this a whirl

@pprice
Copy link
Contributor Author

pprice commented Feb 2, 2020

@Jens-G; CI failed again; I dug through the logs for errors; again appear to be unrelated, 3 fail for the same reason (python upgrade needed?), and 1 for either a cert error or a missing ruby gem?

6172.3, 6172.4, 6172.5

/usr/bin/python test/test.py --retry-count 5 --features .* --skip-known-failures --server cpp,c_glib,cl,d,java,csharp,py,,rb,hs,perl,php,go,nodejs,dart,erl,lua,rs,netstd,nodets
Traceback (most recent call last):
  File "test/test.py", line 45, in <module>
    assert (cur_version >= req_version), "Python 3.3 or later is required for proper operation."
AssertionError: Python 3.3 or later is required for proper operation.
Makefile:1129: recipe for target 'crossfeature' failed
make: *** [crossfeature] Error 1
RET=$?
if [ $RET -ne 0 ]; then
  cat test/log/unexpected_failures.log
fi
cat: test/log/unexpected_failures.log: No such file or directory

6172.9

/usr/bin/python test/test_sslsocket.py
........ERROR:thrift.transport.TSSLSocket:Error while accepting from ('127.0.0.1', 42976)
Traceback (most recent call last):
  File "/thrift/src/lib/py/build/lib.linux-x86_64-2.7/thrift/transport/TSSLSocket.py", line 382, in accept
    client = self._wrap_socket(plain_client)
  File "/thrift/src/lib/py/build/lib.linux-x86_64-2.7/thrift/transport/TSSLSocket.py", line 184, in _wrap_socket
    server_hostname=self._server_hostname)
  File "/usr/lib/python2.7/ssl.py", line 353, in wrap_socket
    _context=self)
  File "/usr/lib/python2.7/ssl.py", line 601, in __init__
    self.do_handshake()
  File "/usr/lib/python2.7/ssl.py", line 830, in do_handshake
    self._sslobj.do_handshake()
SSLError: [SSL: TLSV1_ALERT_UNKNOWN_CA] tlsv1 alert unknown ca (_ssl.c:590)
PROTOCOL_SSLv3 is not available
PROTOCOL_SSLv2 is not available
Installing binding_of_caller 0.8.0 with native extensions
An error occurred while installing byebug (11.1.1), and Bundler cannot continue.
Make sure that `gem install byebug -v '11.1.1'` succeeds before bundling.
Makefile:651: recipe for target 'all-local' failed

@stale
Copy link

stale bot commented Apr 2, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 2, 2020
@emmenlau
Copy link
Member

emmenlau commented Apr 2, 2020

Dear stale bot, please keep this issue open!

@stale
Copy link

stale bot commented Apr 2, 2020

This issue is no longer stale. Thank you for your contributions.

@stale stale bot removed the wontfix label Apr 2, 2020
@Jens-G
Copy link
Member

Jens-G commented Apr 3, 2020

Theoretically we have a clean CI. Could you rebase this?

@Jens-G
Copy link
Member

Jens-G commented May 30, 2020

Let's give this another try. May I ask to rebase and force-push again?

@Jens-G
Copy link
Member

Jens-G commented Jun 4, 2020

Ping

pprice added 2 commits June 6, 2020 12:41
 - [js] Missing class name for generated services and processors; this makes instrumentation of outbound calls (e.g. which service are we calling in to) difficult. Generating a class name for es6 enables x.constructor.name to get generated name.
 - [js] Fix case where services have a method with an argument named "params", generated code generates an identifier in the "send_XXX" body named "params" which conflicts with the argument. Ensure the identifier in the body is uniquely named.
 - [js] Ensure that derived services (extends XYZService) correctly call super() in their constructor.
 - [ts] Ensure that derived services (extends XYZSerice) correctly specify the base class Client and Processor name
 - [js] Ensure that derived service clients do not redefine private fields (output, pClass, _seqId, _reqs) and call super();
 - [ts] Ensure derived service clients and processors do not redefine private fields in declaration.
 - [js] Missing class name for generated services and processors; this makes instrumentation of outbound calls (e.g. which service are we calling in to) difficult. Generating a class name for es6 enables x.constructor.name to get generated name.
 - [js] Fix case where services have a method with an argument named "params", generated code generates an identifier in the "send_XXX" body named "params" which conflicts with the argument. Ensure the identifier in the body is uniquely named.
 - [js] Ensure that derived services (extends XYZService) correctly call super() in their constructor.
 - [ts] Ensure that derived services (extends XYZSerice) correctly specify the base class Client and Processor name
 - [js] Ensure that derived service clients do not redefine private fields (output, pClass, _seqId, _reqs) and call super();
 - [ts] Ensure derived service clients and processors do not redefine private fields in declaration.
@pprice
Copy link
Contributor Author

pprice commented Jun 6, 2020

Okay; rebased on upstread, merged and force pushed. Let's give this one more go.

@pprice pprice closed this Jun 6, 2020
@pprice pprice reopened this Jun 6, 2020
@Jens-G
Copy link
Member

Jens-G commented Jun 9, 2020

That looks related:

../node_modules/.bin/html-validator --file=gen-html/index.html --verbose
Error: Validator returned unexpected statuscode: 503
at Request._callback (/thrift/src/node_modules/html-validator/index.js:32:23)
at Request.self.callback (/thrift/src/node_modules/request/request.js:185:22)
at Request.emit (events.js:198:13)
at Request. (/thrift/src/node_modules/request/request.js:1161:10)
at Request.emit (events.js:198:13)
at IncomingMessage. (/thrift/src/node_modules/request/request.js:1083:12)
at Object.onceWrapper (events.js:286:20)
at IncomingMessage.emit (events.js:203:15)
at endReadableNT (_stream_readable.js:1145:12)
at process._tickCallback (internal/process/next_tick.js:63:19)

@Jens-G Jens-G self-requested a review June 12, 2020 19:36
@Jens-G Jens-G removed their request for review June 12, 2020 19:38
@Jens-G Jens-G changed the title Fix a number of js/ts generation issues THRIFT-5234 Fix a number of js/ts generation issues Jun 17, 2020
@Jens-G
Copy link
Member

Jens-G commented Jun 20, 2020

commited 7db2d0f

@Jens-G Jens-G closed this Jun 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants