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

Remove ember-rfc176-data patch #1

Closed
buschtoens opened this issue Aug 10, 2020 · 7 comments · Fixed by #12
Closed

Remove ember-rfc176-data patch #1

buschtoens opened this issue Aug 10, 2020 · 7 comments · Fixed by #12

Comments

@buschtoens
Copy link
Contributor

buschtoens commented Aug 10, 2020

The required Ember._memo mapping for { memo } from '@glimmer/tacking' has not yet been added to ember-rfc176-data. This is why we use patch-package to patch this in, just so we can get our feet off the ground. For a "stable" release, we need to remove this hackery and land the required change upstream.

These commits need to be reverted:

@buschtoens
Copy link
Contributor Author

I opened ember-cli/ember-rfc176-data#322 to add the mapping.

buschtoens added a commit that referenced this issue Aug 10, 2020
While we are waiting for #1, this will allow us to actually publish a functional, albeit über-hacky, first release. 🚀
@buschtoens
Copy link
Contributor Author

Unfortunately it seems that (at least with yarn) the postinstall script is not (properly) invoked and the patch for ember-rfc-176-data included in this package is not applied in the host project. To be honest, this is probably for the better any way. 😂

I tried patching the package in index.js as well, like so:

function patchRFC176Data() {
  const mapping = require("ember-rfc176-data");
  const hasMemoMapping = Boolean(mapping.find(
    (m) => m.module === "@glimmer/tracking" && m.export === "memo"
  ));
  if (hasMemoMapping) return;
  mapping.push({
    global: "Ember._memo",
    module: "@glimmer/tracking",
    export: "memo",
    deprecated: false,
  });
}

However, this does not work, as ember-cli-babel spawns its workers in child processes, which have their own require.cache. 🙁

@buschtoens
Copy link
Contributor Author

Seems like another option is to add an afterInstall blueprint in which we ask the user, whether they would like us to setup up the patch-package hack in their repo.

Another, possibly much cleaner option could be adding this export to ignore and then transpiling it ourselves.

@buschtoens buschtoens mentioned this issue Aug 11, 2020
4 tasks
@mydea
Copy link
Contributor

mydea commented Aug 17, 2020

Hmm, when installing this locally via yarn, it worked, without applying the patch I believe. (at least the import did not work, I used it directly from Ember._cached). However, now on our CI on Github Actions, it is failing on the install step yarn --frozen-lockfile with:

[5/5] Building fresh packages...
error /.../node_modules/ember-cached-decorator-polyfill: Command failed.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Exit code: 1
Command: patch-package
Arguments: 
Directory: /.../node_modules/ember-cached-decorator-polyfill
Output:
patch-package 6.2.2
Applying patches...
Error: Patch file found for package ember-rfc176-data which is not present at node_modules/ember-rfc176-data

I did nothing but ember install this addon, and it is weird that it works locally with yarn but not on CI. But I guess it is somehow related to this issue here.

@buschtoens
Copy link
Contributor Author

buschtoens commented Aug 17, 2020

Yeah, it seems that patch-package is behaving weirdly. :/

I would like to find some hack to completely avoid it. The most probable solution would be disabling the transpilation of the @glimmer/tracking import, and then either transpiling it ourselves (to avoid issues with embroider's resolution system) or registering a faux @glimmer/tracking module via require.define().

I can push and release a fix, that would at least disable the patch-package stuff when this package is installed as a dependency. Then you can patch yourself or use Ember._cached and CI should work again.

@mydea
Copy link
Contributor

mydea commented Aug 17, 2020

That would work for me, at least :) So if that is OK, then I'd be all for that!

@buschtoens
Copy link
Contributor Author

I've removed patch-package and replaced it with some ember-cli-babel h4xx in #12.

It's released in v0.1.0-3. 🚀

For reference I also raised ember-cli/babel-plugin-ember-modules-api-polyfill#133 to improve the underlying infrastructure for future polyfills.

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 a pull request may close this issue.

2 participants