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: Export object based classes on entry #7822

Merged

Conversation

jmillan
Copy link
Contributor

@jmillan jmillan commented Feb 9, 2023

Along with the non object ones, for consistency. This is a regression introduced recently.

Before:
export { UpdateSettingsRequest } from './worker/update-settings-request.js';

Now:
export { UpdateSettingsRequest, UpdateSettingsRequestT } from './worker/update-settings-request.js';

Along with the non object ones, for consistency. This is a regression
introduced recently.

Before:
 `export { UpdateSettingsRequest } from './worker/update-settings-request.js';`

Now:
 `export { UpdateSettingsRequest, UpdateSettingsRequestT } from './worker/update-settings-request.js';`
@github-actions github-actions bot added c++ codegen Involving generating code from schema javascript typescript labels Feb 9, 2023
jmillan added a commit to versatica/mediasoup that referenced this pull request Feb 9, 2023
This reverts commit 6c10e82.

Until the following regression fix is implemented:

google/flatbuffers#7822
@jmillan jmillan mentioned this pull request Feb 9, 2023
6 tasks
@jmillan
Copy link
Contributor Author

jmillan commented Feb 10, 2023

I'll fix Generated Code checks.

@jmillan
Copy link
Contributor Author

jmillan commented Feb 11, 2023

CodeQL action failing. I've run locally and I don't get any error. I can't re-rerun the action, probably due to permissions in the repo, in case this was due to some temporal error.

@dbaileychess, what do you think about this PR? Object based classes were before exported along with non object ones. I consider their absence a regression, and good to have both exported at the same level.

@bjornharrtell
Copy link
Collaborator

Sorry for forgetting about the object API when reworking the export logic. The fix looks good to me, thanks.

@jmillan
Copy link
Contributor Author

jmillan commented Feb 16, 2023

Hi,

Any chance this PR can be reviewed?

@jmillan
Copy link
Contributor Author

jmillan commented Feb 24, 2023

Please, can this PR have few minutes for review? It fixes a regression which makes otherwise add a great amount of import statements in user code.

@bjornharrtell confirms the issue and considers the PR OK.

I'd like to avoid changing my code so much due to the issue that is fixed here.

@bjornharrtell
Copy link
Collaborator

Ping @dbaileychess

@dbaileychess dbaileychess merged commit 3e778ac into google:master Mar 3, 2023
dbaileychess added a commit that referenced this pull request Mar 15, 2023
* TS/JS: Export object based classes on entry

Along with the non object ones, for consistency. This is a regression
introduced recently.

Before:
 `export { UpdateSettingsRequest } from './worker/update-settings-request.js';`

Now:
 `export { UpdateSettingsRequest, UpdateSettingsRequestT } from './worker/update-settings-request.js';`

* only export object based classes for structs

Enums are not elegible.

---------

Co-authored-by: Björn Harrtell <bjornharrtell@users.noreply.github.com>
Co-authored-by: Derek Bailey <derekbailey@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ codegen Involving generating code from schema javascript typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants