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

Move ember-cli-build to tests/dummy for addons #7907

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
3 participants
@astronomersiva
Copy link
Contributor

commented Jul 9, 2018

Addresses #7486

@astronomersiva

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

@kellyselden

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

We must add some tests for this. We have to make sure the old location still works (and the new location is in fact being used). But this great work.

@astronomersiva

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

Sure, I will try to get some tests written for this. I also wanted your opinion on this.

I believe this if check is no longer needed as ember-cli-build.js is going to be present at the project root for both addons and apps. Will returning projectRoot be sufficient here?

@astronomersiva astronomersiva force-pushed the astronomersiva:master branch 2 times, most recently from 7ae4672 to bf85546 Jul 12, 2018

@astronomersiva

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

@kellyselden I gave a try at writing some tests...please take a look.

@astronomersiva astronomersiva force-pushed the astronomersiva:master branch 3 times, most recently from 2f0cc25 to 129ae9c Jul 13, 2018

if (!buildFilePath) {
return null;
}

process.chdir(path.dirname(buildFilePath));
if (isEmberCLIAddon) {
process.chdir(projectRoot);

This comment has been minimized.

Copy link
@kellyselden

kellyselden Jul 16, 2018

Member

It seems like we could do process.chdir(projectRoot); in both cases and remove the if


packageJson.dependencies = packageJson.dependencies || {};
// add HTMLBars for templates (generators do this automatically when components/templates are added)
packageJson.dependencies['ember-cli-htmlbars'] = 'latest';

This comment has been minimized.

Copy link
@kellyselden

kellyselden Jul 16, 2018

Member

Can't this be put into the fixtures?

This comment has been minimized.

Copy link
@astronomersiva

astronomersiva Jul 16, 2018

Author Contributor

Tried it initially but started getting node_modules is empty errors. Since this is all handled by the acceptance test helpers, I looked around and found this in a few other places.


expect(result.code).to.eql(0);

let cssPath = path.join(addonRoot, 'addon', 'styles', 'app.css');

This comment has been minimized.

Copy link
@kellyselden

kellyselden Jul 16, 2018

Member

Why check css for an ember-cli-build test?

This comment has been minimized.

Copy link
@astronomersiva

astronomersiva Jul 16, 2018

Author Contributor

I wanted to ensure that the options of ember-cli-build are being respected(in this case, fingerprinting) when the file is placed at both locations. No specific reason as to why I chose app.css and not some other file.

it('works when ember-cli-build is at addon root', co.wrap(function *() {
yield copyFixtureFiles('addon/ember-cli-build-at-root');

// remove ember-cli-build.js created at addon root

This comment has been minimized.

Copy link
@kellyselden

kellyselden Jul 16, 2018

Member

Comment seems wrong

// remove ember-cli-build.js created at addon root
let dummyPath = path.join(addonRoot, 'tests', 'dummy', 'ember-cli-build.js');
if (fs.existsSync(dummyPath)) {
fs.unlinkSync(dummyPath);

This comment has been minimized.

Copy link
@kellyselden

kellyselden Jul 16, 2018

Member

Why would this file be here? Doesn't this mean the fixtures are wrong?

This comment has been minimized.

Copy link
@astronomersiva

astronomersiva Jul 16, 2018

Author Contributor

This is because of us creating test targets in the before hook. This results in an ember-cli-build.js being present in tests/dummy because of my changes and since we run find-up from that directory for an addon, that file's contents are taken for this test which isn't desirable.

@kellyselden

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

you also have a change to an experiments file that probably was an accident

@astronomersiva astronomersiva force-pushed the astronomersiva:master branch 2 times, most recently from 37df071 to 4d415bf Jul 16, 2018

@astronomersiva

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

@kellyselden I have made some changes and left a few comments..Please take a look when you are free.

@astronomersiva

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

A gentle reminder @kellyselden 😃

@astronomersiva

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

@kellyselden , I would really appreciate if you take a look at this 😄

@rwjblue
Copy link
Contributor

left a comment

If we go down this path (and are not super careful) we are closing the door on having multiple dummy apps.

I think I'd prefer to have this be configuration (that could be specified in package.json of the addon to override the default of ./ember-cli-build.js), along with a --build-file command line argument to ember build / ember serve / ember test.

return uniq(appFiles.concat(addonFiles));
// There will be two ember-cli-build.js present when app and addon
// files are concatenated - one at tests/dummy and project root.
let i = files.indexOf('ember-cli-build.js');

This comment has been minimized.

Copy link
@rwjblue

rwjblue Jul 25, 2018

Contributor

I think I would tweak the assignment above:

let files = [].concat(
  appFiles.filter(f => f !== 'ember-cli-build.js'),
  addonFiles,
);

Instead of splicing manually...

// for addons, the ember-cli-build.js file will be at tests/dummy.
// find-up will only find files upwards in the directory.
// To solve this, resolve the path for ember-cli for Ember Addons.
options.cwd = path.resolve(process.cwd(), `${projectRoot}/tests/dummy`);

This comment has been minimized.

Copy link
@rwjblue

rwjblue Jul 25, 2018

Contributor

I don't like this hard-coding, it makes (for example) having multiple dummy apps impossible...

@astronomersiva astronomersiva force-pushed the astronomersiva:master branch from 4d415bf to 004fdd6 Sep 1, 2018

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.