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

Deprecate %external extension #6906

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Jul 23, 2024

Resolves #6905

Added new extensions that can replace undocumented feature %external.

  • Added Js.globalThis API -> separated into Add Js.globalThis object binding #6909
  • %global to make globalThis referencing easy and replaces most of the existing %external use cases.
  • %define is compatible with bundlers' define plugin, but with a guard condition. (same behavior with existing %external, but with sensible semantic)
  • Deprecated %external.

Does this change make sense?

@cometkim cometkim changed the title deprecate %exteranl in favor of %global and %define deprecate %external in favor of %global and %define Jul 23, 2024
@cometkim
Copy link
Member Author

cometkim commented Jul 23, 2024

Ok It seems having %global does not make sense except for compatibility. Js.globalThis["x"] as option should be enough.

And having %external is... not very useful. The define plugin is mainly used for configuration and DCE. Adding guard code increases complexity and compromises bundler compatibility.

For example %define(process.env.NODE_ENV) === "production", user don't want guard code for it.

@cknitt
Copy link
Member

cknitt commented Jul 24, 2024

I would prefer getting rid of %external and just using bindings or %raw instead, like:

@scope("globalThis")
external window: option<Dom.window> = "window"

@cristianoc @zth what do you think?

@cometkim
Copy link
Member Author

I would prefer getting rid of %external and just using bindings or %raw instead, like:

Agreed for %global semantic. But I think %define semantic can be useful for defined identifiers like process.env.NODE_ENV since we don't make sure to make it always inlined as-is.

@cknitt
Copy link
Member

cknitt commented Jul 24, 2024

In our projects I am using a binding

type t = [#development | #production]

@scope(("process", "env")) external env: t = "NODE_ENV"

which works fine for me.

if NodeEnv.env === #development { ... }

is compiled to

if (process.env.NODE_ENV === "development") { ... }

@cometkim
Copy link
Member Author

Ok then, I will change this PR to adding globalThis binding and getting rid of %external (with an explicit error), with no additional PPXs

@cknitt
Copy link
Member

cknitt commented Jul 24, 2024

Ok then, I will change this PR to adding globalThis binding and getting rid of %external (with an explicit error), with no additional PPXs

I think you will need to wait for a new rescript-react release without %external, otherwise the playground won't build.

@cometkim
Copy link
Member Author

Ok then just deprecation warnings for v12.0

@cometkim cometkim changed the title deprecate %external in favor of %global and %define Deprecate %external extension Jul 24, 2024
@cometkim cometkim marked this pull request as ready for review July 24, 2024 18:04
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Is it deprecated or entirely removed?

@cometkim
Copy link
Member Author

Is it deprecated or entirely removed?

Currently just print a deprecation warning (%external is deprecated, use %raw or regular FFI syntax instead.)

Because there are some actual usage. Particularly it is used inside some official libraries like @rescript/react. So I think if they drop support for v11 after a major update, then we can remove the feature entirely. Maybe in v12.x

@cometkim
Copy link
Member Author

@cknitt any comment?

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@cknitt cknitt merged commit 3b47ef9 into rescript-lang:master Jul 25, 2024
19 checks passed
@cometkim cometkim deleted the external-ppx branch July 25, 2024 06:53
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.

RescriptReactRouter crash on master (window vs. $$window)
3 participants