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

[TS/JS] Entry point per namespace and reworked 1.x compatible single file build #7510

Merged
merged 51 commits into from Jan 21, 2023

Conversation

bjornharrtell
Copy link
Collaborator

@bjornharrtell bjornharrtell commented Sep 5, 2022

Implements reworked entry point generation, so that each namespace gets an entrypoint and re-exports children. The top level entry point can be bundled up into a single file that should be 1.x compatible. This also has the benefit of avoiding name clashes which occured in the IMHO not so well thought out previous attempts (including my own) to export everything in a single module at root level. A new option is added --ts-entry-points to enable the entry point generation which also implicitly also enables --gen-all because it needs all symbols for the entry points.

Note that this PR also rolls back the change to the npm package entry point name. It was changed to js/index.js some time ago but is now back to js/flatbuffers.js which it was before (also in earlier 2.x versions) because IMHO that was an unnesseary breaking change. Specifically this makes it possible to run the old test suite from flatbuffers 1.x which is added back as tests/ts/JavaScriptTestv1.cjs and it works with no other modification than moving from flatbuffers.Long to native BigInt support which was was a breaking change we made years ago. One could argue we should have kept flatbuffers.Long for backwards compatibility but that ship has sailed and probably not worth the effort anyway IMHO.

The previous implementation of option --ts-flat-files has been replaced by esbuild automation. It simply automates runs of esbuild to produce old style 1.x generation single file bundles that assume flatbuffers is available in global scope.

This PR also removes some left over older generated TS/JS files missed in #7508.

References #7448.

TODO

  • Handle reserved words (currently not replaced on re-export)
  • Check for esbuild availability when --ts-flat-files is used
  • Fix/update flatc_ts_tests.py
  • Handle case with root namespace table with same name as fbs

@github-actions github-actions bot added c++ CI Continuous Integration codegen Involving generating code from schema grpc javascript json python typescript labels Sep 5, 2022
@bjornharrtell bjornharrtell changed the title Ns entry points [TS/JS] Entry point per namespace Sep 5, 2022
@bjornharrtell

This comment was marked as outdated.

@bjornharrtell

This comment was marked as outdated.

@github-actions github-actions bot added json python and removed CI Continuous Integration python json grpc labels Oct 2, 2022
@github-actions github-actions bot added the CI Continuous Integration label Oct 4, 2022
@bjornharrtell bjornharrtell force-pushed the ns-entry-points branch 2 times, most recently from 6c21594 to 15f6d38 Compare October 5, 2022 19:53
@bjornharrtell bjornharrtell changed the title [TS/JS] Entry point per namespace [TS/JS] Entry point per namespace and reworked single file build Oct 5, 2022
@bjornharrtell bjornharrtell changed the title [TS/JS] Entry point per namespace and reworked single file build [TS/JS] Entry point per namespace and reworked 1.x compatible single file build Oct 5, 2022
@bjornharrtell bjornharrtell marked this pull request as ready for review October 5, 2022 20:02
@bjornharrtell
Copy link
Collaborator Author

bjornharrtell commented Oct 5, 2022

I've pushed through with this and moved it out of draft for review. It produces valid single file output that is 1.x compatible for the schemas in tests/ts. However, the flatc ts tests failures remains unresolved. I remain convinced what this PR does is worthwile goal and less hacky/quirky than what it replaces so that we only have two kinds of outputs instead of four (!). What I mean with this is that counting 1.x and 2.x current possible JavaScript/TypeScript output you can get 1.x compat single file builds, my initial attempted "flat" namespace entry points, your (@dbaileychess) initial --ts-flat-files and the "modern" individual modular output introduced with the TS rewrite. With this PR we can go back provide only 1.x compatible single file builds and the individual modular output which is what I see as the big win here.

While I do agree with the general idea of the flatc tests and what their purpose is I remain unconvinced the flatc ts tests provide real value because of the contrived schemas and that even the current implementation in master does not produce something that is valid JavaScript or TypeScript from such schemas.

@dbaileychess I sincerely hope we can discuss or move forward with this in some way. I've put many many hours of effort into this PR.

@bjornharrtell
Copy link
Collaborator Author

Unfortunately I find it hard to resolve the merge with master now due to #7748. Will make a new attempt when I have some more time.

@bjornharrtell
Copy link
Collaborator Author

I've been able to do the merge now as proper as I can but tests specific to no_import_ext fail like follows:

no_import_ext/optional-scalars.ts:3:30 - error TS2835: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './optional-scalars/optional-byte.js'?

This is expected to me and I don't quite understand how the tests passed in #7748. @ink-su and @sunwen18 do you have any ideas?

@bjornharrtell
Copy link
Collaborator Author

Nevermind I think I understand it now, it needs to be tested using the "old" node resolution mode of typescript. I've separated the tests.

But I see another issue now. When using prefix (-o) it looks like module paths become wrong for no_import_ext but strangely enough not for union_vector... sigh.

@bjornharrtell
Copy link
Collaborator Author

I think I've been able to figure out the corner cases now after merge with master.

@jkuszmaul can you take a look at this again in the context of bazel?

@jkuszmaul
Copy link
Contributor

@bjornharrtell ns-entry-points in my clone of flatbuffers has a single commit that should get bazel working (https://github.com/jkuszmaul/flatbuffers/tree/ns-entry-points).

Unfortunately, we lose typing information because the new esbuild method
of generating single files does not generate type information.

The method used here is a bit hack-ish because it relies on parsing the
console output of flatc to figure out what to do.
@jkuszmaul
Copy link
Contributor

@bjornharrtell I'll take a look at the failing tests... There was some special logic around reflection.fbs that I had previously, but I may be able to rip it out entirely.

@jkuszmaul
Copy link
Contributor

@bjornharrtell cherry-pick cdaff66ed25f79d426a064d0fa9ee0392ad33708 from the same branch (or just merge the branch in, same thing).

@bjornharrtell
Copy link
Collaborator Author

And we're green! Thanks @jkuszmaul! Ping @dbaileychess.

@dbaileychess
Copy link
Collaborator

@bjornharrtell I'm going to to approve this without a thorough review (its 241 files !) so I trust this is the path we want to take.

@jkuszmaul I appreciate your contributions to help this along!

@dbaileychess dbaileychess merged commit ef76b5e into google:master Jan 21, 2023
@domoritz
Copy link

This change seems to have broken apache/arrow builds with closure compiler.

yarn run gulp compile:esnext:umd
yarn run v1.22.19
$ /Users/dominik/Code/arrow/js/node_modules/.bin/gulp compile:esnext:umd
[22:41:01] Using gulpfile ~/Code/arrow/js/gulpfile.js
[22:41:01] Starting 'compile:esnext:umd'...
[22:41:07] 'compile:esnext:umd' errored after 5.27 s
[22:41:07] Error: gulp-google-closure-compiler: targets/esnext/cls/ipc/message.js:18:0:
Originally at:
targets/esnext/cls/ipc/ipc/message.ts:19:0: ERROR - [JSC_DOES_NOT_HAVE_EXPORT] Requested module does not have an export "ByteBuffer".

1 error(s), 0 warning(s)


CustomError: gulp-google-closure-compiler: Compilation errors occurred
    at CompilationStream._compilationComplete (/Users/dominik/Code/arrow/js/node_modules/google-closure-compiler/lib/gulp/index.js:238:28)
    at /Users/dominik/Code/arrow/js/node_modules/google-closure-compiler/lib/gulp/index.js:208:14
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

    at formatError (/Users/dominik/Code/arrow/js/node_modules/gulp-cli/lib/versioned/^4.0.0/format-error.js:21:10)
    at Gulp.<anonymous> (/Users/dominik/Code/arrow/js/node_modules/gulp-cli/lib/versioned/^4.0.0/log/events.js:33:15)
    at Gulp.emit (node:events:524:35)
    at Gulp.emit (node:domain:489:12)
    at Object.error (/Users/dominik/Code/arrow/js/node_modules/undertaker/lib/helpers/createExtensions.js:61:10)
    at handler (/Users/dominik/Code/arrow/js/node_modules/now-and-later/lib/map.js:50:14)
    at f (/Users/dominik/Code/arrow/js/node_modules/once/once.js:25:25)
    at f (/Users/dominik/Code/arrow/js/node_modules/once/once.js:25:25)
    at tryCatch (/Users/dominik/Code/arrow/js/node_modules/bach/node_modules/async-done/index.js:24:15)
    at done (/Users/dominik/Code/arrow/js/node_modules/bach/node_modules/async-done/index.js:40:12)
error Command failed with exit code 1.

it works fine with 23.1.4

@bjornharrtell
Copy link
Collaborator Author

@domoritz it would be good to track that as a new issue and if you can please provide more details and/or instructions how to reproduce. It is not obvious to me what has caused it.

@bjornharrtell bjornharrtell deleted the ns-entry-points branch January 30, 2023 09:11
@domoritz
Copy link

Thank you, yes. I need to dig a bit more into it to identify the real issue. I suspect it's something with the changes in the package.json.

To reproduce, I clone arrow, cd js, run yarn, and run yarn run gulp compile:esnext:umd. This works. Then when I update the flatbuffers dependency, and run yarn && yarn run gulp compile:esnext:umd, I get the failure above. This doesn't feel like a good reproduction because the repo is so large.

@bjornharrtell
Copy link
Collaborator Author

@domoritz I think I have found and reproduced the problem, see #7814.

@domoritz
Copy link

domoritz commented Feb 6, 2023

Thank you for the follow up. I subscribed to the new issue and will try a fixed version when it comes out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ CI Continuous Integration codegen Involving generating code from schema documentation Documentation grpc javascript json python typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants