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

[QUEST] Module Unification: Final Cut #16373

Closed
mixonic opened this issue Mar 13, 2018 · 29 comments

Comments

Projects
None yet
@mixonic
Copy link
Member

commented Mar 13, 2018

Throughout 2017 a small crew has pushed forward the implementation of Ember's new filesystem layout for applications and addons. This was described in RFC 143 and we've lovingly called the effort "module unification".

Much of the effort up to now has been either design or exploration-heavy. It wasn't plausible to reduce the work list into things that individual contributors could pick up without context.

But in the last weeks we have turned a corner. Our effort is behind three feature flags in Ember, Ember CLI, and ember-resolver. In order to enable these flags by default for new applications, we need your help. In this quest issue I've outlined the current list of known blockers as well as guidance for how to test the new features in new and migrated codebases.

Several contributors have been huddled in #st-module-unification on the Ember.js Community Slack in recent months. Please join us there when you start to pick up one of these tasks.

The Issue List

Ember.js

  • In the Ember.js PR that landed namespaces behind a feature flag, we implemented parsing of {{namespace::name}} strings at runtime. This implementation should be improved to have this work happen at Ember app build time.
  • Before advocating to "go" the feature, we want to get benchmarks (via https://github.com/krisselden/ember-macro-benchmark and https://github.com/eviltrout/ember-performance) to confirm there isn't an unexpected large regression. The changes to implement these features are not expected to improve Ember's performance, but we should understand the impact.

Ember CLI

  • Ember CLI uses the presence of src/ to configure the return value of isModuleUnification(). This works well for apps and addons. The detection fails for a classic dummy app in a module unification addon, however. Although a dummy app may be in tests/dummy/app/ with no tests/dummy/src/ directory, the addon's root level src/ confuses our logic and the dummy app is incorrectly treated as a module unification app.
  • Requires design: An ember addon author may want two dummy apps for testing, a module unification src/ app and a classic app/ app. Their acceptance tests would run with both apps, or they could have two acceptance test suites.
  • Module Unification blueprints and generators, mostly tracked at: ember-cli/ember-cli#7530
    • (@cibernox ember-cli/ember-cli#7667) The module unification Ember addon blueprint should generate a module unification dummy app, not a classic app.
    • Generators for initializers and instance-initializers are broken
    • The test_helper.js files for module unification addons (https://github.com/ember-cli/ember-cli/blob/master/blueprints/module-unification-addon/files/tests/test-helper.js#L1) should import from ../src/main.
    • The app and addon dummy app blueprints both use the fallback resolver via resolver.js and classic initializers support via main.js. Before we remove the feature flag, we need to convert each addon these apps are dependent upon to be a module unification addon. Then we should remove the fallback setup for newly generated apps and addon dummy apps.
  • ember-maybe-import-generator-for-testing needs to apply to tests in src directory, see https://github.com/ember-cli/ember-maybe-import-regenerator-for-testing
  • If a classic app is running a version of Ember CLI that pre-dates module unification support, then an addon using module unification (with a src/ directory)must have a way a) fail gracefully or b) bring the reexport logic we use in ember-cli with it to the old version of ember-cli.
  • Module unification addons re-export their top level components to app/ for classic apps. This allows an addon author to use a src/ directory and still have their addon work for either kind of app. We would like to provide addons a build-time way to configure this re-export logic. It would permit them to re-name components and services to match legacy classic APIs while they adopt new APIs for module unification apps.
  • Ember addons need an API that allows them to alter the module unification config (for example https://github.com/ember-cli/ember-resolver/blob/master/mu-trees/addon/ember-config.js) and add their own collections and types. In theory Ember (or the resolver?) would use this same API to add its own types and collections.
  • Fix lib/models/project.js's isModuleUnification method to not only rely on presence of top level src as "the project is module unification format". Specifically, an addon may be in module unification format, but have its dummy app be in classic format.

Ember Template Compiler

  • hbs helpers should accept a source argument like templates in a running app do. In: https://github.com/ember-cli/ember-cli-htmlbars-inline-precompile
  • Part of the hbs AST transform should be to inline the source value. However test files for a private collection, local lookup component might not be able to use the "easy" value of their filename and still have access to resolve their subject. For example in src/ui/routes/application/-component/foo-bar/component-test.js templates created with hbs will not be able to access the {{foo-bar}} component. The simplest solution here is to pass a source to these hbs templates that is "one level up", for example the source for templates in the test file would be src/ui/routes/application. There are some tradeoffs in taking this or other approaches, but it seems like a good place to start. ember-cli/babel-plugin-htmlbars-inline-precompile#33

Ember Resolver

Migrator

  • (@iezer rwjblue/ember-module-migrator#73) The migrator should update the test_helper.js file to import the app from ../src/main. Currently this mentioned in the fallback documentation but it should just be part of the conversion.
  • The migrator currently renames template-only components to not have a folder, like src/ui/components/name.hbs. The resolver doesn't yet support this, and instead always expects a folder, for example: src/ui/components/name/template.hbs. The ember-resolver can probably be tweaked to fix this at runtime, but alternatively the migrator should simply not do this flattening in the near-term.
  • Migrator tries to re-export with the name helper, but also imports a thing with the name helper. #16361

ember-svg-jar

  • This addon is the only one we've found with a src/ directory that is not used for module unification. We need to update it for compatibility: ivanvotti/ember-svg-jar#56

ember-load-initializers

  • Test files in the initializers directory should be ignored by the loader. We can't strip test files from development output since
    ce it would break /tests/ on the dev server, but we can easily have the initializer ignore them. Tracked at ember-cli/ember-load-initializers#47

Several contributors have been huddled in #st-module-unification on the Ember.js Community Slack in recent months. Please join us there when you start to pick up one of these tasks.

Creating a new module unification app

Install Ember CLI master:

npm install -g https://github.com/ember-cli/ember-cli.git

Generate a new app with the module unification env variable:

MODULE_UNIFICATION=true EMBER_CLI_MODULE_UNIFICATION=true ember new my-app

Out of the box a newly generated application will use the ember-resolver "fallback" resolver. This allows the module unification app to use classic app/ addons. Much later, once we've converted the default addons that ship with new Ember apps, we can drop the "fallback" resolver and use the MU-only "glimmer wrapper" resolver.

The implementation of module unification uses a configuration of collections and types. In the classic addon app/ layout, addons could define new types of factories without any explicit definition. Before removing the feature flags we need an API for them to explicitly do so against the module unification config. Temporarily you may want to mutate the config for your app to add a type, for example in src/resolver.js.

If you have an issue after generating a new application you can likely find or file an issue:

Migrating an existing Ember app

Install the module migrator:

npm install -g ember-module-migrator jscodeshift

Run the migrator on your app codebase:

cd ~/project/path
ember-module-migrator

Install Ember CLI master:

npm install --save-dev https://github.com/ember-cli/ember-cli.git

And update your files to match the latest blueprint, similar to what you do for any upgrade:

ember init

You'll want to:

  • Keep the ember-resolver feature flag in config/environment.js.
  • Keep the Ember.js feature flag in config/environment.js.
  • Keep the canary version of Ember.js in package.json.
  • Keep the addition of loadInitializers in src/main.js. There are two of these lines, one for src/ and one for classic addons exporting into app/.
  • Keep the change to the fallback resolver in src/resolver.js.
  • Keep the change to import the app from ../src/main in test-helper.js.

If you have an issue after migrating an application you can likely find or file an issue:

Creating a new module unification addon

Install Ember CLI master:

npm install -g ember-cli/ember-cli#master

Generate a new app with the module unification env variable:

MODULE_UNIFICATION=true ember addon my-addon

You can find a file issues for the experience of building an addon:

@cibernox

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2018

@mixonic The task that has a lock and my handle is also done.

@acorncom

This comment has been minimized.

Copy link
Member

commented Mar 16, 2018

Thanks @cibernox I’ve noted that

@cibernox

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2018

@acorncom You can also check The test_helper.js files for module unification addons...

@acorncom

This comment has been minimized.

Copy link
Member

commented Mar 16, 2018

@cibernox done. As other items get done, feel free to ping me here or on Slack to help keep the ball rolling 👍

@GavinJoyce GavinJoyce referenced this issue Mar 23, 2018

Open

Module Unification: Blueprints #7530

18 of 29 tasks complete
@iezer

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

The checkbox about ember-maybe-import-generator-for-testing can be closed, see ember-cli/ember-maybe-import-regenerator-for-testing#2

@iezer

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

@rwjblue I looked at the 2nd migrator issue:

"The migrator currently renames template-only components to not have a folder, like src/ui/components/name.hbs."

And I'm not sure that it's a real issue. More context here: rwjblue/ember-module-migrator#72

Do you know if there is a specific case where this is an issue?

@rwjblue

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

No, it was likely something fixed when pods support was added to the migrator...

@iezer

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

Cool in that case that box can also be checked.

@iezer

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

The first migrator task is fixed by this PR rwjblue/ember-module-migrator#73

@ivanvotti

This comment has been minimized.

Copy link

commented Mar 29, 2018

It is fixed in the latest ember-svg-jar v1.X.X -- the src/ directory has been renamed.

@mixonic

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2018

Thanks @iezer for the fix and @ivanvotti for adjusting the ember-svg-jar codebase for the greater ecosystem 👏 🕺

@efx

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

@mixonic ember-cli/ember-load-initializers#47 should also be tracked here. I have a precursory PR that would need to be merged before taking a stab to fix this. I will try to verify this still happens today.

@mixonic

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2018

@efx added!

@mansona

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

Myself and @iezer may have solved both items in the "Ember Template Compiler" section above 🤔 I think we may have skipped the first checkbox and moved straight onto the auto-interpretation ember-cli/babel-plugin-htmlbars-inline-precompile#33

@efx

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

@mixonic ember-cli/ember-load-initializers#47 should be fixed now thanks to @GavinJoyce .

@GavinJoyce

This comment has been minimized.

Copy link
Member

commented May 17, 2018

The instructions for creating a MU app have changed, @mixonic perhaps you could update the quest issue description with these instructions?

Creating a new module unification app

Install Ember CLI master:

npm install -g https://github.com/ember-cli/ember-cli.git

Generate a new app with the module unification env variables:

MODULE_UNIFICATION=true EMBER_CLI_MODULE_UNIFICATION=true ember new my-mu-app

Generate a component:

EMBER_CLI_MODULE_UNIFICATION=true ember g component x-button

Run the app:

EMBER_CLI_MODULE_UNIFICATION=true ember serve
@GavinJoyce

This comment has been minimized.

Copy link
Member

commented May 26, 2018

^ I've updated the instructions to create and run a MU app (EMBER_CLI_MODULE_UNIFICATION=true is needed when running ember serve). Perhaps someone who has edit access to the issue description could update the instructions provided in the comment above?

@cyk

This comment has been minimized.

Copy link

commented May 30, 2018

Fix lib/models/project.js's isModuleUnification method to not only rely on presence of top level src as "the project is module unification format". Specifically, an addon may be in module unification format, but have its dummy app be in classic format.

So, a Blueprint's "module unification mode" is currently determined by the root Project's isModuleUnification (i.e., the presence of a "/src".). But the root project is unaware of the generated destination's format (e.g., tests/dummy/app). If a Blueprint knows enough to detect the destination's format (and maybe a future --dummy "custom"), would it make sense to give Blueprint it's own isModuleUnification?

Edit: Simplified.

@karthiicksiva

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2018

^ I've updated the instructions to create and run a MU app (EMBER_CLI_MODULE_UNIFICATION=true is needed when running ember serve). Perhaps someone who has edit access to the issue description could update the instructions provided in the comment above?

@mixonic Can you please update the issue description for ember s?

I didn't read after the description and missed @GavinJoyce comment. Thanks.

@mixonic

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

@karthiicksiva yeah I (or someone else) needs to re-confirm all the instructions above in light of the new options.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

Some questions about the outstanding tasks -

  • for someone new to any particular code base, where would I need to begin / what would I need to know to solve some of the tasks?

In the Ember.js PR that landed namespaces behind a feature flag, we implemented parsing of {{namespace::name}} strings at runtime. This implementation should be improved to have this work happen at Ember app build time.

What is involved in doing something at build time? where does this happen in ember.js? Where is the current parsing of {{namespace::name}} located?

If a classic app is running a version of Ember CLI that pre-dates module unification support, then an addon using module unification (with a src/ directory)must have a way a) fail gracefully or b) bring the reexport logic we use in ember-cli with it to the old version of ember-cli.

is this actionable? or is this just a thing to make sure we're doing later on?

Module unification addons re-export their top level components to app/ for classic apps. This allows an addon author to use a src/ directory and still have their addon work for either kind of app. We would like to provide addons a build-time way to configure this re-export logic. It would permit them to re-name components and services to match legacy classic APIs while they adopt new APIs for module unification apps.

what all does this involve?

hbs helpers should accept a source argument like templates in a running app do. In: https://github.com/ember-cli/ember-cli-htmlbars-inline-precompile

this one I've looked at before, and I could not figure out where / how the entry point worked.
Because usage is like:

layout = hbs`{{some-template}}`

and I know there is some connection with the babel transform for htmlbars.
My confusion could be just from being new to template functions, but any additional guidance / more specifics here would be super beneficial :)

:)

also, should we use a github project for managing remaining to-dos?

@aalasolutions

This comment has been minimized.

Copy link

commented Oct 10, 2018

Hmm, not sure where to put these, but for me writing EMBER_CLI_MODULE_UNIFICATION=true each time is hassle. I have changed my package.json file with these

"scripts": {
    "build": "EMBER_CLI_MODULE_UNIFICATION=true ember build",
    "lint:hbs": "ember-template-lint .",
    "lint:js": "eslint .",
    "start": "EMBER_CLI_MODULE_UNIFICATION=true ember serve",
    "g": "EMBER_CLI_MODULE_UNIFICATION=true ember g",
    "d": "EMBER_CLI_MODULE_UNIFICATION=true ember d",
    "test": "EMBER_CLI_MODULE_UNIFICATION=true ember test"
  },

and now I can just run npm run g component todo-item and it will generate properly ...

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2018

those can all be removed now. check this out: https://github.com/ember-cli/ember-cli/pull/8157/files

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2018

Some work on multiple dummy apps / isModuleUnification detection here: ember-cli/ember-cli#8158

@buschtoens

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

@NullVoxPopuli #16373 (comment):

In the Ember.js PR that landed namespaces behind a feature flag, we implemented parsing of {{namespace::name}} strings at runtime. This implementation should be improved to have this work happen at Ember app build time.

What is involved in doing something at build time? where does this happen in ember.js? Where is the current parsing of {{namespace::name}} located?

The OP includes a link to the PR and file that implements this. ☺️

https://github.com/emberjs/ember.js/pull/16319/files#diff-80f190cc4372dfc7275150e4b47cbd6bR272

Very relevant RFC: #367: "Module Unification Packages (MU with Ember Addons)"

@buschtoens

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

I found a bug with nested routes. You can't go deeper than two levels: #17217

@buschtoens

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

Found another one: #17224. Addon tests are hoisted into the consuming host application and run by ember test.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

I hope that's related to an issue I saw the other day. Where tests remain in the prod build

@mixonic

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

In light of the updates at these links:

...I don't believe this quest issue is sure to be a valid work list. I'm closing it and I'm sure we will create new issue at the appropriate time.

@mixonic mixonic closed this Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.