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

Add test for v2 addon importing from v1 addon's fake module #1101

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Feb 3, 2022

This added test is to try to recreate some troubles I've been experiencing with

The end goal is to reproduce the problem in embroider's test suite, so I can debug what's going on a little easier.

@ef4
Copy link
Contributor

ef4 commented Feb 3, 2022

Either way this will be good to keep in the test suite.

@ef4
Copy link
Contributor

ef4 commented Feb 3, 2022

I think your original bug may have been around @glimmer/component, which is definitely different from the other "fake" ember packages because a real package does exist, but the implementation inside it is really only for standalone glimmerX and shouldn't actually be used, because Ember contains the real implementation.

@NullVoxPopuli
Copy link
Collaborator Author

I think your original bug may have been around @glimmer/component

yeah, I have a couple things I'm trying to reproduce:

from here: https://github.com/NullVoxPopuli/ember-addon-v2-typescript-demo
@glimmer/component appears to be undefined.

from here: NullVoxPopuli/ember-resources#366

@josemarluedke
Copy link
Collaborator

josemarluedke commented Feb 3, 2022

I was playing with ember-addon-v2-typescript-demo and figured out that @glimmer/component is returning an object with default instead of the component class.

See:
Screen Shot 2022-02-03 at 2 21 13 PM

Screen Shot 2022-02-03 at 2 21 08 PM

When logging Component, we can see it as an object:

Screen Shot 2022-02-03 at 2 21 26 PM

If I change the dist files to class FlatDemo extends Component.default, that works.

EDIT: That setup was not using embroider actually, just ember-auto-import.

@ef4
Copy link
Contributor

ef4 commented Feb 4, 2022

The problem in that demo repo is:

https://github.com/NullVoxPopuli/ember-addon-v2-typescript-demo/blob/125700fc9aa77faec4d9a49c514a4fa90e477996/my-addon/package.json#L10

When webpack sees that it gets Very Strict with your module, including disabling things like interoperability between import and require.

But we need the interoperability, because in a classic build (which is what that repo is using), @glimmer/component has long since been transpiled from an ES module to AMD by the time our v2 addon gets it. Without the interop, we don't know to grab the .default property off the value that comes back from require().

Under Embroider things are different, because webpack will see @glimmer/component as a real ES module. But the build fails in that case for other reasons. I'm not really sure if we can turn on this level of Webpack strictness until much more of Ember itself ships as ES modules. We liberally use require() in the compiled output to paper over the gap between Ember's runtime loader and ES module loading.

@ef4
Copy link
Contributor

ef4 commented Feb 4, 2022

I think you can use Rule.type to override webpack's behavior here and go back from javascript/esm to javascript/auto.

@NullVoxPopuli
Copy link
Collaborator Author

ah, interesting -- I hadn't heard of rule.type before.

Changing my config in the test app to this has everything working with @josemarluedke's addon-dev changes. 🎉

updated emebr-cli-build.js
'use strict';

const EmberApp = require('ember-cli/lib/broccoli/ember-app');

module.exports = function (defaults) {
  let app = new EmberApp(defaults, {
    autoImport: {
      watchDependencies: [Object.keys(require('./package').dependencies)],
      webpack: {
        devtool: 'inline-source-map',
        module: {
          rules: [{ test: /\.(js|ts)$/, type: 'javascript/auto' }],
        },
      },
    },
  });

  const { maybeEmbroider } = require('@embroider/test-setup');

  return maybeEmbroider(app, {
    packagerOptions: {
      webpackConfig: {
        devtool: 'inline-source-map',
        module: {
          rules: [{ test: /\.(js|ts)$/, type: 'javascript/auto' }],
        },
      },
    },
  });
};

Some followup questions though -- and maybe "elsewhere" is more appropriate for this sort of discussion, but idk:

Without the interop, we don't know to grab the .default property off the value that comes back from require().

does the problem go away if we convert @glimmer/component to a v2 addon? (I know that's not easy and has a lot of nuance right now, since all the implementations of stuff are actually in glimmer -- but just conceptually -- is it the whole ecosystem that needs this sort of interop? or just the @glimmer/* stuff?

I'm not really sure if we can turn on this level of Webpack strictness until much more of Ember itself ships as ES modules. We liberally use require() in the compiled output to paper over the gap between Ember's runtime loader and ES module loading.

does a PR to flip to the defaults to javascript/auto in both auto-import and embroider make sense? (at least until more native ES Modules are shipped? (also, does this mean it's more than just @glimmer/* that has some special interop? (I suppose the template-compiler?)))

NullVoxPopuli added a commit to NullVoxPopuli/ember-addon-v2-typescript-demo that referenced this pull request Feb 4, 2022
@NullVoxPopuli NullVoxPopuli force-pushed the try-v2-addon-with-glimmer-cache-primitives-import branch from f7d8a31 to fd94c6e Compare February 26, 2022 04:11
@NullVoxPopuli NullVoxPopuli force-pushed the try-v2-addon-with-glimmer-cache-primitives-import branch from f468bd1 to 4f5d507 Compare April 6, 2022 16:25
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.

3 participants