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

Bring back deprecated String prototype extensions #24101

Closed

Conversation

chancancode
Copy link
Contributor

@chancancode chancancode commented Oct 25, 2023

Brings back: "foo".camelize() => "Foo" + "please import from `@ember/string"

A bit of a brain-twister. This apparently stopped working after #24034

In 3.28, @ember/string is one of the magic modules provided by ember-source, just like @ember/component etc, as opposed to a real package you install.

Later on in the 4.x cycle that was extracted into a real package so the code can be deprecated and removed from core, and you only ship those bytes if your app opt-into it.

We are on 3.28, but we also installed the real package version. This is fine, the real package doesn't do anything so fancy that it wouldn't work with 3.28 apps, we are just duplicating the code in the payload but otherwise it's alright.

However, the real package version, extracted in 4.x, does not add the prototype extensions, which was deprecated in 3.x for removal by 4.0, so depending on which version (the ember-source one or the real package one), there is that difference in behavior.

The layout of the bundles looks something like this:

// vendor.js

// ember-source
define("@ember/string/index", [...], function (exports, ...deps) {
  // first copy of @ember/string
  exports.camelize = function camelize() {};

  String.prototype.camelize = withDeprecation(camelize);
});

define("@ember/component/index", ["@ember/string", ...], function (exports, ...deps) {
  // ...
});
// one of the webpack chunks
webpack.modules.push({
  "something-something-implicit-modules": () => {
    define("@ember/string", [], function () {
      return __webpack__require__("node_modules/@ember/string/index.js");
    });

    // ...
  },
  "node_modules/@ember/string/index.js": () => {
    // second copy of @ember/string
    __webpack_exports__.camelize = function camelize() {};

    // no prototype extensions here
  }
});

I think what ends up happening in this situation is that Webpack compiled modules end up using the real package version, and addons etc, will load it via loader.js, and depending on their luck, one of the two defined()-ed version gets picked up.

However, prior to #24034, we have a patch that ends up causing something to this effect to be appended to the end of vendor.js:

// bottom of vendor.js
(function() {
  require("@ember/component");
})();

I think that patch was to work around some build issues of the module shims, but because the require happens synchronously inside vendor.js, it ends up deterministically running the version of @ember/string with the prototype extensions and ensured any loader.js consumers will see that version of the module.

The patch was removed with #24034, and so that stopped working reliably as reported in https://meta.discourse.org/t/topic-list-previews-theme-component/209973/376

Ordinarily the solution is probably to just remove the package version for now, but Embroider also seems to assum that everyone is already on the package version, so a patch is needed for that to work.

All of these needs to be reverted when we upgrade Ember. Or we can just accept not having the prototype extensions since they are already deprecated anyway (and the slight cost of shipping two copies of that code).

"foo".camelize() => `"Foo"` + "please import from `@ember/string"

A bit of a brain-twister. This apparently stopped working after

In 3.28, `@ember/string` is one of the magic modules provided by
`ember-source`, just like `@ember/component` etc,  as opposed to
a real package you install.

Later on in the 4.x cycle that was extracted into a real package
so the code can be deprecated and removed from core, and you only
ship those bytes if your app opt-into it.

We are on 3.28, but we _also_ installed the real package version.
This is fine, the real package doesn't do anything so fancy that
it wouldn't work with 3.28 apps, we are just duplicating the code
in the payload but otherwise it's alright.

However, the real package version, extracted in 4.x, does not add
the prototype extensions, which was deprecated in 3.x for removal
by 4.0, so depending on which version (the `ember-source` one or
the real package one), there is that difference in behavior.

The layout of the bundles looks something like this:

```js
// vendor.js

// ember-source
define("@ember/string/index", [...], function (exports, ...deps) {
  // first copy of @ember/string
  exports.camelize = function camelize() {};

  String.prototype.camelize = withDeprecation(camelize);
});

define("@ember/component/index", ["@ember/string", ...], function (exports, ...deps) {
  // ...
});
```

```js
// one of the webpack chunks
webpack.modules.push({
  "something-something-implicit-modules": () => {
    define("@ember/string", [], function () {
      return __webpack__require__("node_modules/@ember/string/index.js");
    });

    // ...
  },
  "node_modules/@ember/string/index.js": () => {
    // second copy of @ember/string
    __webpack_exports__.camelize = function camelize() {};

    // no prototype extensions here
  }
});
```

I think what ends up happening in this situation is that Webpack
compiled modules end up using the real package version, and addons
etc, will load it via loader.js, and depending on their luck, one
of the two `defined()`-ed version gets picked up.

However, prior to discourse#24034, we have a patch that ends up causing
something to this effect to be appended to the end of `vendor.js`:

```js
// bottom of vendor.js
(function() {
  require("@ember/component");
})();
```

I think that patch was to work around some build issues of the
module shims, but because the require happens synchronously inside
`vendor.js`, it ends up deterministically running the version of
`@ember/string` with the prototype extensions and ensured any
loader.js consumers will see that version of the module.

The patch was removed with discourse#24034, and so that stopped working
reliably as reported in https://meta.discourse.org/t/topic-list-previews-theme-component/209973/376

Ordinarily the solution is probably to just remove the package
version for now, but Embroider also seems to assum that everyone
is already on the package version, so a patch is needed for that
to work.

All of these needs to be reverted when we upgrade Ember. Or we can
just accept not having the prototype extensions since they are
already deprecated anyway (and the slight cost of shipping two
copies of that code).
@chancancode chancancode force-pushed the bring-back-string-prototype-ext branch from 6d33fb4 to 63d4e09 Compare October 25, 2023 19:26
So when we switch back to the real package plugins
will continue to have access.
davidtaylorhq added a commit that referenced this pull request Oct 26, 2023
These have been deprecated for some time, and the vast majority of themes/plugins have already removed their use. The prototype extensions were unexpectedly disabled as a side effect of 895036b (more details in #24101).

Given that restoring the functionality now involves significant complexity, and would only be delaying the inevitable removal in a matter of months, we've decided to keep them disabled. This commit explicitly sets the flag in the ember environment config to make things clearer.
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/removal-of-ember-string-prototype-extensions/283513/4

davidtaylorhq added a commit that referenced this pull request Oct 26, 2023
These have been deprecated for some time, and the vast majority of themes/plugins have already removed their use. The prototype extensions were unexpectedly disabled as a side effect of 895036b (more details in #24101).

Given that restoring the functionality now involves significant complexity, and would only be delaying the inevitable removal in a matter of months, we've decided to keep them disabled. This commit explicitly sets the flag in the ember environment config to make things clearer.
@davidtaylorhq
Copy link
Member

Thanks for the investigation here @chancancode 🙏

Or we can just accept not having the prototype extensions since they are already deprecated anyway (and the slight cost of shipping two copies of that code).

Yeah, I think that makes the most sense for us. Handled via #24110, and we have a topic about it on Meta here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants