embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
337 stars 136 forks source link

Unable to import from a v1 addon in a v2 addon #1179

Closed runspired closed 2 years ago

runspired commented 2 years ago

I have a v2 addon that defines several transitions for liquid-fire, which it lists as a peerDependency. Attempting to build results in the following error:

!] Error: 'isAnimating' is not exported by ../../../node_modules/liquid-fire/index.js, imported by src/transitions/move-over-half.js
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
src/transitions/move-over-half.js (2:26)
1: import $ from "jquery";
2: import { animate, finish, isAnimating, Promise, stop } from "liquid-fire";
                             ^
3: 
4: function biggestSize(context, dimension) {
Error: 'isAnimating' is not exported by ../../../node_modules/liquid-fire/index.js, imported by src/transitions/move-over-half.js

I would have expected that @embroider/addon-dev helped with this situation out of the box. In my plugins I list as expected addon.dependencies() which the code comments led me to believe was for this purpose.

runspired commented 2 years ago

This is the inverse of #1175 in case that wasn't clear 😅

runspired commented 2 years ago

I was able to resolve this by using a better rollup config. I'd initially tried mirroring some things that apollo-glimmer had done, but shifted to doing what ember-animated had done since ember-animated also ships colocated components written in typescript. NVM I got trolled by a different build error making it seem like I'd gotten past this, I had not :(

NullVoxPopuli commented 2 years ago

Can you share your configs, before and after?

NullVoxPopuli commented 2 years ago

I think maybe related, I ran in to an issue with a v2 addon importing from an optional peer dependency declared in an app here: https://github.com/NullVoxPopuli/ember-resources/runs/5842084950?check_suite_focus=true#step:7:428

Since ember-async-data needs to be a v2 addon anyway, that's my path forward for that particular issue. But, I think it'd be fairly straight-forward to make a scenario test for this :thinking:

runspired commented 2 years ago

@NullVoxPopuli yes that's the same issue, but it'll be a rough thing for the community to disentangle what has and hasn't been migrated yet if we block migration based on whether deps/peerDeps are also all migrated.

I think the issue might be something like that while rollup is being configured to treat the module as external here: https://github.com/embroider-build/embroider/blob/main/packages/addon-dev/src/rollup-addon-dependencies.ts it is still trying to validate that the module actually exists. Either that or the above config doesn't work when using typescript :/

NullVoxPopuli commented 2 years ago

rough thing for the community to disentangle what has and hasn't been migrated yet if we block migration based on whether deps/peerDeps are also all migrated.

I agree -- that's why I make test :D

it is still trying to validate that the module actually exists.

well, the issue is that we're running in to an app-buildtime problem and that rollup plugin is an addon build-time thing for the v2 addon. So I think something may be goofy with how we're preparing things to send off to webpack (hopefully a failing test will tell us more)

runspired commented 2 years ago

@NullVoxPopuli got this solved locally, you have to pass the addon to rollup as external in the config. It would be nice if the addonDependencies plugin would do this automatically, but it's fairly easy to do manually too. Here's my configs for an addon written in TS that uses components and imports from an external addon in v1.

babel.config.json

{
  "presets": [
    [
      "@babel/preset-typescript"
    ]
  ],
  "plugins": [
    "@embroider/addon-dev/template-colocation-plugin",
    [
      "@babel/plugin-proposal-decorators",
      {
        "version": "legacy"
      }
    ]
  ]
}

rollup.plugin.js

import { Addon } from "@embroider/addon-dev/rollup";
import ts from "rollup-plugin-ts";

const addon = new Addon({
  srcDir: "src",
  destDir: "dist",
});

const globallyAvailable = ["components/**/*.{js,ts}", "helpers/**/*.{js,ts}"];

export default {
  output: addon.output(),

  external: ["liquid-fire"],

  plugins: [
    addon.publicEntrypoints([...globallyAvailable, "transitions/**/*.{js,ts}"]),

    addon.appReexports(globallyAvailable),

    ts({
      transpiler: "babel",
      browserslist: false,
      transpileOnly: true, // private in-mono-repo package, global tsconfig already handles type wiring
      tsconfig: {
        fileName: "../../../tsconfig.json",
        hook: (config) => ({ ...config, declaration: true }),
      },
    }),

    addon.dependencies(),

    addon.hbs(),

    addon.keepAssets(["**/*.css"]),

    addon.clean(),
  ],
};
runspired commented 2 years ago

Also the dependencies setup here:

  "dependencies": {
    "@embroider/addon-shim": "^1.5.0"
  },
  "peerDependencies": {
    "liquid-fire": "^0.34.0"
  },
  "devDependencies": {
    "@embroider/addon-dev": "^1.5.0",
    "rollup": "^2.70.1",
    "@babel/core": "^7.17.8",
    "@babel/plugin-proposal-class-properties": "^7.16.7",
    "@babel/plugin-proposal-decorators": "^7.17.8",
    "@babel/plugin-transform-typescript": "^7.16.8",
    "@babel/plugin-transform-runtime": "^7.17.0",
    "@babel/preset-typescript": "^7.16.7",
    "@babel/preset-env": "^7.16.11",
    "@babel/runtime": "^7.17.8",
    "@rollup/plugin-babel": "^5.3.1",
    "rollup-plugin-ts": "^2.0.5",
    "typescript": "^4.6.3"
  },
NullVoxPopuli commented 2 years ago

ah interesting -- ok -- I'm going to ensure this works over here then: https://github.com/embroider-build/embroider/pull/1126/files#diff-0d15995080ee9f281ca534fe2ec6e630f4770d419cfc99a68f9f3ebcac778e42

thanks for following up!