ember-cli / ember-cli

The Ember.js command line utility.
https://cli.emberjs.com
MIT License
3.26k stars 1.16k forks source link

move ember-cli-build.js to inside dummy folder for addons #7486

Open kellyselden opened 6 years ago

kellyselden commented 6 years ago

Tracking this because I can't immediately jump on this, and would be great for new contributors!

Addons have two build files in their root dir, "index.js" and "ember-cli-build.js". "index.js" is run when an addon is built inside a host app, and "ember-cli.build.js" is the addon's "dummy" app's build file that is only used when running the addon's tests. It is for this reason, I think we should move the file to "tests/dummy/ember-cli-build.js" in the blueprints, and also make ember-cli recognize it at the new location.

villander commented 6 years ago

@kellyselden I was able to move ember-cli-build.js to thetests/dummy, but I get the error ember-cli-build.js found.

Look my PR: https://github.com/ember-cli/ember-cli/pull/7602

findBuildFile uses https://github.com/sindresorhus/find-up

So I do not need to change anything in the builder. But it still does not find ember-cli-build.js inside thetest/dummy folder. I looked for the cause, but it did not make much sense to me.

Another problem is that tests/dummy/ember-cli-build.js is ignored when i push my addon on github. But i don't found nothing about this: https://github.com/villander/villander

villander commented 6 years ago

@Turbo87 can you give me a hint how to solve this please?

Turbo87 commented 6 years ago

I don't know, sorry

rwjblue commented 6 years ago

You need to update the findBuildFile utilty to leverage the nested path when the project is an addon.

The findBuildFile utiltity is used here: https://github.com/ember-cli/ember-cli/blob/87337ee328b4db84e087b8166d698d31ead24b02/lib/models/builder.js#L49

astronomersiva commented 6 years ago

I gave a shot at this. Please take a look.

locks commented 4 years ago

Closing as the PR was also closed. See the discussion in https://github.com/ember-cli/ember-cli/pull/7907 for more details. Thank you @astronomersiva for your help!

kellyselden commented 4 years ago

I don't know why that PR was closed, but I still think this is the right thing to do.

rwjblue commented 4 years ago

Ya, I generally agree. We should move at least move ember-cli-build.js and config/ember-try.js into the dummy app...

Turbo87 commented 4 years ago

config/ember-try.js?

did you mean config/environment.js?

knownasilya commented 3 years ago

@rwjblue which file did you mean?

rwjblue commented 3 years ago

Whoops, missed that ping. Ya, I meant what I had said: ember-cli-build.js should move to tests/dummy/ember-cli-build.js and config/ember-try.js should move to tests/dummy/config/ember-try.js. We should then remove the top level config/environment.js file (it is empty and provides basically no value, yet at the same time massively increases confusion for folks).

SergeAstapov commented 3 years ago

We should then remove the top level config/environment.js file

@rwjblue just double checking, this will not affect standalone ember engines which requires top level config/environment.js file?

rwjblue commented 3 years ago

@SergeAstapov - RE: an addon using config/environment.js, my point was that most addons do not use it, and therefore having that file included in the addon blueprint is just overly confusing (since it has a fundamental different meaning between app and addon). The actual behavior itself wouldn't change though (we already allow the file to be missing for example). Additionally, the engines blueprint could continue to emit the file.

SergeAstapov commented 3 years ago

Thank you for clarification @rwjblue!

saravanak commented 2 years ago

I want to have a go at this.

I have this in blueprints/addon/index.js, which moves the generated ember-cli-build to the correct location, cool!

fileMap: {
....
    '^addon-config/ember-try.js': 'config/ember-try.js',
+    'ember-cli-build.js': 'tests/dummy/ember-cli-build.js',
...

The next piece is to refine findBuildFile to return the correct build file location.

This comment foresees support for multiple dummy applications. Ember addon docs about tests/dummy say

When you run npm test in the addon, this dummy app is also used as the host app, as if the addon were installed into it.

Which, IMO, tells the users that only one dummy app is supported. As part of this issue fix, should we only assume only one dummy app for now, like so

findBuildFilegets path arg as as addon_root/tests/dummy, so as it resolves up, makes the following builds possible:

ember-cli-build.js inside addon_root:

ember-cli-build.js inside addon_root/tests/dummy:

We are hard-coding the tests/dummy, but this is done only in the context of an addon (this.project.isAddon?), and blueprints/addon/index.js already does this hard-coding fwiw.

Suggested change Builder::readBuildFile: (pseduocode follows)

+  path = this.project.isAddon ? path.join(path, 'tests', 'dummy'): path;
    let buildFile = findBuildFile('ember-cli-build.js', path);

I can raise this as a PR for review, but just wanted to get the idea and approach correct before I do so.

locks commented 2 years ago

@saravanak still interested in giving it a go? :)