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

feat(napi/transform): return helpers information #7737

Merged

Conversation

Boshen
Copy link
Member

@Boshen Boshen commented Dec 9, 2024

closes #7599

Copy link

graphite-app bot commented Dec 9, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request labels Dec 9, 2024
Copy link
Member Author

Boshen commented Dec 9, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Boshen Boshen marked this pull request as draft December 9, 2024 08:28
Copy link
Member Author

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

need review

Copy link

codspeed-hq bot commented Dec 9, 2024

CodSpeed Performance Report

Merging #7737 will not alter performance

Comparing 12-09-feat_napi_transform_return_helpers_information (2803aec) with main (065f7dc)

Summary

✅ 29 untouched benchmarks

Copy link
Member Author

Boshen commented Dec 9, 2024

Merge activity

  • Dec 9, 4:35 AM EST: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
  • Dec 9, 9:03 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Dec 9, 9:08 AM EST: A user added this pull request to the Graphite merge queue.
  • Dec 9, 9:12 AM EST: A user merged this pull request with the Graphite merge queue.

@Boshen Boshen force-pushed the 12-09-feat_napi_transform_return_helpers_information branch from d58e349 to d672bd7 Compare December 9, 2024 11:20
@overlookmotel
Copy link
Contributor

overlookmotel commented Dec 9, 2024

I think this needs a separate option HelperLoaderMode::Global. As I understand it from #7599, the requirement is:

  1. Do NOT generate import / require.
  2. Create a unique symbol name for the helper function.
  3. Helper usage includes that symbol name e.g. _defineProperty(...).
  4. Return object mapping symbol names to import paths.

This is different from the existing modes:

  • Runtime mode inserts import/require.
  • External mode doesn't create a symbol name for each helper - it outputs babelHelpers.defineProperty(...) not _defineProperty(...).

HelperLoaderMode::Global should use TraverseCtx::generate_uid_name to generate symbol name, instead of generate_uid_in_root_scope. This will create a unique name and add it to set of used UIDs so it doesn't get used again, but will NOT create a binding. The IdentifierReference returned should be unbound (i.e. global reference).

Also see #7599 (comment). I'm not completely sure this is going to solve the root problem.

@Boshen Boshen force-pushed the 12-09-feat_napi_transform_return_helpers_information branch 2 times, most recently from c8b478b to ff21dfb Compare December 9, 2024 14:02
@Boshen Boshen marked this pull request as ready for review December 9, 2024 14:02
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Dec 9, 2024
@Boshen Boshen force-pushed the 12-09-feat_napi_transform_return_helpers_information branch from ff21dfb to 2803aec Compare December 9, 2024 14:07
@graphite-app graphite-app bot merged commit 2803aec into main Dec 9, 2024
23 checks passed
@graphite-app graphite-app bot deleted the 12-09-feat_napi_transform_return_helpers_information branch December 9, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[transformer] bundle runtime helpers / remove runtime helper import code from output
2 participants