Skip to content
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

Is Typescript totally necessary? #96

Closed
jeffwillette opened this issue Oct 27, 2017 · 18 comments
Closed

Is Typescript totally necessary? #96

jeffwillette opened this issue Oct 27, 2017 · 18 comments
Assignees

Comments

@jeffwillette
Copy link
Contributor

I am wondering if this can be made to use with plain ES5 javascript without the typescript bits. I tried for a while to integrate it in a normal ReactJS workflow but I could not do it without TS

@MarcusLongmuir
Copy link
Contributor

@deltaskelta: Can you please explain the issues you were seeing? Apologies for the long delay in responding to your issue.

@jeffwillette
Copy link
Contributor Author

I have since given up and gone with pure typescript, but I just fired up an example with create-react-app and I get multiple compile errors saying 'proto' is not defined from the _pb.js.

for example...

Line 292:  'proto' is not defined     no-undef

which is quite obviously coming from the lines...

var jspb = require('google-protobuf');
var goog = jspb;
var global = Function('return this')();

goog.exportSymbol('proto.pb.PBObject', null, global);

So I am unsure if the issue is coming form the way I am compiling the files, which is just as prescribed from the improbable-eng repo, or if I should look elsewhere for the reason why it is not finding the symbol after exportSymbol is called.

@easyCZ
Copy link
Contributor

easyCZ commented Nov 23, 2017

Hey @deltaskelta, would you be able to provide a minimal set of steps to reproduce the issue? Thanks

@jeffwillette
Copy link
Contributor Author

Yes I will try, it might be a few days until I can get around to it.

@jeffwillette
Copy link
Contributor Author

jeffwillette commented Nov 24, 2017

I made a repo that simply recreates the issue...
https://github.com/deltaskelta/grpc-web-issue-96

If you follow the simple instructions in the README and start the react dev server, you can see the errors.

the proto files and the compile script are in react-app/src/protobuf so you can see how they were compiled

@easyCZ
Copy link
Contributor

easyCZ commented Nov 27, 2017

@deltaskelta Thanks for the repo. Looking into it! There's also a mention of this issue on

protocolbuffers/protobuf#3931

@easyCZ easyCZ self-assigned this Nov 27, 2017
@jonathan-se
Copy link

When I packed the react project with webpack it resolves all the imports and the result is a working bundle.js.

Not, sure but I believe the problem has something to do with the server provided by the create-react-app and with its way of resolving imports and requires.

Still the webpack solution isn't ideal for the developing process itself, so a better solution would be great!

@easyCZ
Copy link
Contributor

easyCZ commented Nov 28, 2017

I came to a similar finding yesterday, it looks to be related to create-react-app rather. A non create-react-app setup works fine for me. Will dig deeper into what's triggering this in create-react-app.

@jeffwillette
Copy link
Contributor Author

What do you mean by "packed"? do you mean you ejected the webpack config from the stock create-react-app and modified it?

I suspected it might be a webpack issue but I tried some different loaders and didn't have much luck.

@jonathan-se
Copy link

I created a webpack config myself and after webpacking it I used the output bundle.js and an index.html with a different server than the create-react-app one.

@jeffwillette
Copy link
Contributor Author

jeffwillette commented Nov 28, 2017

ok, could you post the webpack config here? or a gist?

@easyCZ
Copy link
Contributor

easyCZ commented Nov 28, 2017

I can confirm that there are no issues when using webpack outside of create-react-app. Here's a grpc-web-js-examle repo with a working pure JS usage of grpc-web. Will continue to investigate why create-react-app throws errors.

@jeffwillette
Copy link
Contributor Author

jeffwillette commented Nov 29, 2017

ok thanks I'll look into it too

@jeffwillette
Copy link
Contributor Author

I figured out what was causing it in create-react-app... It is this line.

https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/config/webpack.config.dev.js#L138

It is only failing to compile because the default react-scripts forces eslint errors to fail to compile. It is part of create-react-app's opinionated webpack config. I can see only two ways to go about fixing it

  1. The scripts can be ejected, which I believe removes the line above by default
  2. /* eslint-disable */ can be added to the generated js file

Thanks for your time in helping troubleshoot this issue

@jonathan-se
Copy link

jonathan-se commented Dec 4, 2017

It seems this problem in create-react-app is wider than the protobuf issue.
See related "doesn't respect .eslintignore" issue - https://github.com/facebookincubator/create-react-app/issues/2339)

@jeffwillette
Copy link
Contributor Author

@jonathan-se your link text shows the right place, but it actually links right to this thread

@xense
Copy link

xense commented Aug 21, 2019

Here is my bash script.

First loop generates .js and .ts files.
Second loop adds /* eslint-disable */' to the beginning of generated files.

TS out is not necessary here, but it helps a lot in IDE.

for f in "${PROTO_DIR}"/*.proto
do
    protoc3 \
      --plugin="protoc-gen-ts=${PROTOC_GEN_TS_PATH}" \
      --proto_path="${PROTO_DIR}" \
      --js_out=import_style=commonjs,binary:${OUT_DIR} \
      --ts_out=${OUT_DIR} \
      "${f}"
done


for f in "${OUT_DIR}"/*.js
do
    echo '/* eslint-disable */' | cat - "${f}" > temp && mv temp "${f}"
done

@BichengWang
Copy link

That's so interesting. above two solution work for me. Is it possible to add alla generated js files to a specific folder and setting react lint ignore?

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

No branches or pull requests

6 participants