embroider-build / embroider

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

Non-JS files in mirage dir are ignored #758

Open ef4 opened 3 years ago

ef4 commented 3 years ago

We seem to have had a regression recently in apps using typescript with ember-cli-mirage. Any TS files in the mirage subdir are getting dropped.

ef4 commented 3 years ago

I will point out that what ember-cli-mirage does here -- emitting these files in its own treeForApp, was strictly-speaking never allowed, because an addon is not allowed to have files in treeForApp that require transpilation. It just happens to usually work for ember-cli-mirage because the files came originally from the app.

But an addon taking some of the app's files and re-emitting them into the app is a weird pattern that needs to stop anyway.

I think once we upgrade to webpack 5 we can just tell apps to setup exports in package.json like:

"exports": {
  "./*": "./app/*",
  "./mirage/*": "./mirage/*"
}

And then both app and mirage will be resolvable using normal resolving rules, and mirage can stop touching the app's /mirage directory at all.

ef4 commented 3 years ago

This was introduced in 0.38.0, so people can workaround by staying on 0.37.0 until we have a fix.

billpull commented 3 years ago

Does this issue lead to the following error?

Module not found: Error: Can't resolve '../mirage/config' in '$TMPDIR\embroider\402607\initializers\ember-cli-mirage.js'

stephencweiss commented 3 years ago

@billpull - yes, it seems that way! @ef4 actually answered a very similar question for me within the Ember Discord

smketterer commented 3 years ago

Just wanted to mention that I think this issue also crops up with GraphQL files as well, e.g. if you're using @mirage/graphql and have your schema in your mirage folder:

Module not found: Error: Can't resolve './schema.graphql' in '$TMPDIR/embroider/ae5643/mirage/config.js'

Downgrading to 0.37.0 fixes this specifically.

knownasilya commented 3 years ago

Saw the same as @smketterer, and not using TS in the mirage dir.

Screen Shot 2021-04-29 at 2 14 03 PM
ef4 commented 3 years ago

Yes, this issue could be better described as "non-JS files in mirage dir are ignored".

chrismllr commented 2 years ago

Hey all, currently pinning my version of embroider due to this, is there a current workaround, or a suggested webpack configuration addition to handle these files? Or does this necessitate a fix upstream for mirage?

wagenet commented 2 years ago

This seems to be an acceptable workaround in our app:

  let appTree = new WatchedDir('app');

  if (env !== 'production') {
    appTree = new MergeTrees([
      appTree,
      new Funnel(new WatchedDir('mirage'), {
        destDir: 'mirage',
      }),
    ]);
  }

  let app = new EmberApp(defaults, {
    trees: { app: appTree },

    // ...

EDIT 2: The above code actually seems to work as desired.

chrismllr commented 2 years ago

This seems to be an acceptable workaround in our app:

  let trees = [];

  if (env !== 'production') {
    let mirage = new Funnel('mirage', {
      destDir: 'mirage',
    });
    trees.push(mirage);
  }

  compatBuild(app, Webpack, {
    extraPublicTrees: trees,
  });

EDIT: Actually, this only seems to work for allowing builds to succeed. Factories don't appear to be correctly picked up by mirage for use in server.create.

Will give it a shot! We are taking the approach of using a "classic" build in all environments but production, but it'd be nice to start bringing our local dev/ testing environments into the embroider fold.

wagenet commented 2 years ago

@chrismllr I updated my above comment with something that seems to actually do the trick!

chrismllr commented 2 years ago

@wagenet

  let app = new EmberApp(defaults, {
    trees: { app: appTree },

    // ...

EDIT 2: The above code actually seems to work as desired.

Was the intention to pass this to new EmberApp, or to pass to the compatBuild?

wagenet commented 2 years ago

@chrismllr to EmberApp.

chrismllr commented 2 years ago

Yep, can confirm, builds are working! Seeing an additional error popping up when actually serving the app in the browser, that might be ember auto import related?:

loader.js:247 Uncaught Error: Could not find module `miragejs` imported from `(require)`
    at missingModule (loader.js:247)
    at findModule (loader.js:258)
    at requireModule (loader.js:24)
    at eval (miragejs.js?b62d:2)
    at Object.../../../externals/miragejs.js (chunk.29690000496779c524ef.js:9649)
    at __webpack_require__ (chunk.ce21ec05a1eb4db712e6.js:31)
    at eval (activity-item.ts:5)
    at Module../mirage/factories/activity-feed-item.ts (chunk.29690000496779c524ef.js:8419)
    at __webpack_require__ (chunk.ce21ec05a1eb4db712e6.js:31)
    at Module.eval [as callback] (ui.js?a263:140)

Appears that miragejs is being added as an external in webpack somewhere, not sure if thats intentional


/***/ "../../../externals/miragejs.js":
/*!**************************************!*\
  !*** ../../../externals/miragejs.js ***!
  \**************************************/
/***/ ((module) => {

eval("const m = window.require(\"miragejs\");\n\nif (m.default && !m.__esModule) {\n  m.__esModule = true;\n}\n\nmodule.exports = m;//# sourceURL=[module]\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,...\n//# sourceURL=webpack-internal:///../../../externals/miragejs.js\n");

EDIT: versions:

{
    "@embroider/compat": "0.47.2",
    "@embroider/core": "0.47.2",
    "@embroider/router": "^0.47.2",
    "@embroider/webpack": "0.47.2",
    "@miragejs/graphql": "^0.1.11",
    "ember-auto-import": "^2.2.3",
    "ember-cli-mirage": "^2.1.0",
}
wagenet commented 2 years ago

@chrismllr definitely possible, I never actually got my build entirely working because of some import issues in another library. There may still be issues with my suggested solution that I just haven't noticed yet!

chrismllr commented 2 years ago

Some progress; I've realized that @embroider & ember-auto-import@v2 are more explicit about declaring in your package.json any package you intend on importing modules from. Above, I was trying to use miragejs, but it wasn't in my dependencies (it was available by way of ember-cli-mirage). Webpack looks to have declared it an "external" package since if it wasn't in my package.json.

By changing my import:

// before
import { Model } from 'miragejs';

// after
import { Model } from 'ember-cli-mirage';

Its now working correctly.

I am using the exact snippet from https://github.com/embroider-build/embroider/issues/758#issuecomment-983059421 and everything seems to be working đź‘Ť

SergeAstapov commented 2 years ago

I will point out that what ember-cli-mirage does here -- emitting these files in its own treeForApp, was strictly-speaking never allowed, because an addon is not allowed to have files in treeForApp that require transpilation. It just happens to usually work for ember-cli-mirage because the files came originally from the app.

But an addon taking some of the app's files and re-emitting them into the app is a weird pattern that needs to stop anyway.

I think once we upgrade to webpack 5 we can just tell apps to setup exports in package.json like:

"exports": {
  "./*": "./app/*",
  "./mirage/*": "./mirage/*"
}

And then both app and mirage will be resolvable using normal resolving rules, and mirage can stop touching the app's /mirage directory at all.

as @cah-briangantzler and me are working towards a new major release of ember-cli-mirage, we can do some breaking changes that would enable embroider support out of the box and without extra hacks.

@ef4 would you recommend to do anything else in the ember-cli-mirage itself except a) "stop re-emitting files" and b) adding a step in installation instruction adding "./mirage/*" entry to exports in package.json (which may be automated)?

SergeAstapov commented 2 years ago

Some progress; I've realized that @embroider & ember-auto-import@v2 are more explicit about declaring in your package.json any package you intend on importing modules from. Above, I was trying to use miragejs, but it wasn't in my dependencies (it was available by way of ember-cli-mirage). Webpack looks to have declared it an "external" package since if it wasn't in my package.json.

By changing my import:

// before
import { Model } from 'miragejs';

// after
import { Model } from 'ember-cli-mirage';

Its now working correctly.

I am using the exact snippet from #758 (comment) and everything seems to be working đź‘Ť

Please note next major version of ember-cli-mirage will remove such re-exports so you will need to use import { Model } from 'miragejs'; style and add miragejs to your devDependencies (conceptually similar to what ember-qunit does).

cah-brian-gantzler commented 2 years ago

I think once we upgrade to webpack 5 we can just tell apps to setup exports in package.json like:

"exports": {
  "./*": "./app/*",
  "./mirage/*": "./mirage/*"
}

And then both app and mirage will be resolvable using normal resolving rules, and mirage can stop touching the app's /mirage directory at all.

The above config would place the files in the app ALL the time. Which may be desirable when developing and you dont have a backend yet. Most uses though are for tests only. What would be the config for that?

Part of what ember-cli-mirage does is discover the files, bundle them in the hash that mirageJs expects. Ember-cli-mirage will still need to touch the files in the /mirage directory, unless we go the route that all files will need to be manually imported in the config.js and the user bundle the files themselves

cah-brian-gantzler commented 2 years ago

There is also the method provides that creates the needed models for Mirage from the ember data models (It does not create files that need to be resolved, just the definitions that are passed to mirageJS). So the addon will still need to look at the apps files in the models dir.

Trying to understand some direction on what this addon can provide in an embroider world, or will you just have to do all the boilerplate work yourself to use mirageJS

ef4 commented 2 years ago

The above config would place the files in the app ALL the time.

That’s not correct. With embroider anything you don’t import doesn’t get into your build. If you only import the mirage files from tests, they will only be in tests.

(There are exceptions for backward compatibility, where we deliberately add imports for things, but those can all be turned off.)

On Sat, Feb 5, 2022 at 11:22 AM Brian Gantzler @.***> wrote:

I think once we upgrade to webpack 5 we can just tell apps to setup exports in package.json like:

"exports": { "./": "./app/", "./mirage/": "./mirage/"}

And then both app and mirage will be resolvable using normal resolving rules, and mirage can stop touching the app's /mirage directory at all.

The above config would place the files in the app ALL the time. Which may be desirable when developing and you dont have a backend yet. Most uses though are for tests only. What would be the config for that?

Part of what ember-cli-mirage does is discover the files, bundle them in the hash that mirageJs expects. Ember-cli-mirage will still need to touch the files in the /mirage directory, unless we go the route that all files will need to be manually imported in the config.js and the user bundle the files themselves

— Reply to this email directly, view it on GitHub https://github.com/embroider-build/embroider/issues/758#issuecomment-1030653356, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACN6MWYFMJYNNRICBPU2CLUZVFEPANCNFSM42VPGKPA . You are receiving this because you were mentioned.Message ID: @.***>

ef4 commented 2 years ago

So the addon will still need to look at the apps files in the models dir.

ember-cli-mirage already does that at runtime, not build time: https://github.com/miragejs/ember-cli-mirage/blob/46511c35c9eb23ebfabbb897a5816105fa372af0/addon/ember-data.js#L25

And this always works even under embroider because ember-data itself can only load models from the runtime loader.

But I agree that going into the future we will eventually want to change this, because people don't really want ember-data to be stuck eagerly loading every possible model into the app before it's actually needed. Once ember-data gains the capability to lazily load models, ember-cli-mirage will need a different strategy.

There are multiple possibilities. Generally they would involve a line or two of setup that's under application author control, which I am strongly convinced is better than striving for zero lines of setup, because it gives users a thread to follow when they're learning how things work and because it avoids violating abstraction barriers.

villander commented 1 year ago

Please note next major version of ember-cli-mirage will remove such re-exports so you will need to use import { Model } from 'miragejs'; style and add miragejs to your devDependencies (conceptually similar to what ember-qunit does).

@SergeAstapov it does not work for me.

ro0gr commented 7 months ago

I've just faced this issue, and luckily the Broccoli trick suggested above has saved my day. I'm wondering at this point, what's the path forward here?

Is it an embroider or ember-cli-mirage issue now, when webpack 5 seems to be in its place already?

knownasilya commented 7 months ago

I'm assuming the path forward is getting rid of ember-cli-mirage and using https://miragejs.com/ directly?

ef4 commented 7 months ago

Getting rid of the built-time parts of ember-cli-mirage: yes, definitely.

But probably keeping some of the runtime parts of ember-cli-mirage that smooth over integration between miragejs and ember.

I used mirage as an illustrative example in this RFC to explain one alternative way to let apps send the contents of their /mirage directory into mirage for auto-discovery purposes, without needing any custom build code in ember-cli-mirage.

cah-brian-gantzler commented 7 months ago

While you could use mirageJS directly, There are few things that ember-cli-mirage still gives you. Bascially the helpers to startMirage and SetupMirage for tests. Those relevant parts were extracted and placed in https://github.com/bgantzler/ember-mirage. I am no longer using ember-cli-mirage. The discovering of models was not really embroider friendly. It only took a short time to just create the mirage models and import them. They are making changes in ECM to support the changes being made in ED, but even those changes are for ED Legacy. ED does not look like it will have models going forward and will also allow the schema to be defined at runtime. Any changes made to auto discover models is only a stop gap. Thats why I personally dropped the auto discover and just created the models in mirage.

Mirage is suppose to represent your backend, and reproducing your front end as your backend (auto discover models) isnt really representing your back end. If you have no serializers (you made your front end look like your backend) this is ok I guess. But if you have a lot of serializers (because your backend does not look like your front end) this auto discover just made things more confusing.