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

adding support for addons that use treeFor() to suppress various trees #566

Merged
merged 2 commits into from
Oct 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 50 additions & 6 deletions packages/compat/src/v1-addon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,11 @@ export default class V1Addon {
}

@Memoize()
private hasStockTree(treeName: string) {
private hasStockTree(treeName: string): boolean {
if (this.suppressesTree(treeName)) {
return false;
}

// we need to use this.addonInstance.root instead of this.root here because
// we're looking for the classic location of the stock tree, and that
// location is influenced by a customized ember-addon.main in package.json,
Expand Down Expand Up @@ -360,7 +364,7 @@ export default class V1Addon {
}
}

protected stockTree(treeName: string) {
protected stockTree(treeName: string): Tree {
return this.throughTreeCache(treeName, 'stock', () => {
// adjust from the legacy "root" to our real root, because our rootTree
// uses our real root but the stock trees are defined in terms of the
Expand Down Expand Up @@ -492,6 +496,40 @@ export default class V1Addon {
return tree;
}

// In general, we can't reliably run addons' custom `treeFor()` methods,
// because they recurse in a way that we absolutely don't want.
//
// But there is a very common use case that we *can* handle opportunisticaly,
// which is a treeFor() that's used purely to guard whether `_super` will be
// called or not.
private suppressesTree(name: string): boolean {
if (!this.customizes('treeFor')) {
return false;
}
let origSuper = this.addonInstance._super;
try {
this.addonInstance._super = returnMarkedEmptyTree;
let result = this.mainModule.treeFor?.call(this.addonInstance, name);
if (result === markedEmptyTree) {
// the method returns _super unchanged, so tree is not suppressed and we
// understand what's going on
return false;
}
if (result == null) {
// the method nulled out the tree, so we are definitely suppressing
return true;
}
// we can't tell what's really going on, don't suppress and hope for the
// best
unsupported(`${this.name} has a custom treeFor() method that is doing some arbitrary broccoli processing.`);
return false;
} finally {
if (this.addonInstance._super === returnMarkedEmptyTree) {
this.addonInstance._super = origSuper;
}
}
}

protected invokeOriginalTreeFor(
name: string,
{ neuterPreprocessors } = { neuterPreprocessors: false }
Expand All @@ -505,6 +543,9 @@ export default class V1Addon {
return tree;
};
}
if (this.suppressesTree(name)) {
return undefined;
}
return this.addonInstance._treeFor(name);
} finally {
if (neuterPreprocessors) {
Expand Down Expand Up @@ -856,10 +897,6 @@ export default class V1Addon {
};
}

if (this.customizes('treeFor')) {
unsupported(`${this.name} has customized treeFor`);
}

this.buildTreeForAddon(built);
this.buildAddonStyles(built);
this.buildTreeForStyles(built);
Expand Down Expand Up @@ -919,3 +956,10 @@ function babelPluginAllowedInStage1(plugin: PluginItem) {
function notColocatedTemplate(path: string) {
return !/^\.\/components\/.*\.hbs$/.test(path);
}

const markedEmptyTree = new UnwatchedDir(process.cwd());
const returnMarkedEmptyTree = {
Copy link
Collaborator

@thoov thoov Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ef4 I ran into an issue with this on an addon that calls this._super instead of this._super.treeFor. Currently the way we are mocking this as a POJO results them getting an error with: Uncaught TypeError: this._super is not a function. Obviously, this can easily be fixed by changing their call to this._super.treeFor but not sure how wide spread calling this._super is or if that is even something we should support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, yeah, we can support that. It looks like core-object supports both, which is... ugh.

I filed #569

treeFor() {
return markedEmptyTree;
},
};
68 changes: 68 additions & 0 deletions packages/compat/tests/stage1.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,58 @@ describe('stage1 build', function () {
});
merge(movedMain.pkg, { 'ember-addon': { main: 'custom/index.js' } });

// an addon that uses treeFor() to sometimes suppress its stock trees
let suppressed = app.addAddon(
'suppressed',
`
treeFor(name) {
if (name !== 'app') {
return this._super.treeFor(name);
} else {
return undefined;
}
}
`
);
merge(suppressed.files, {
addon: {
'addon-example.js': '// example',
},
app: {
'app-example.js': '// example',
},
});

// an addon that uses treeFor() to sometimes suppress its custom trees
let suppressedCustom = app.addAddon(
'suppressed-custom',
`
treeFor(name) {
if (name !== 'app') {
return this._super.treeFor(name);
} else {
return undefined;
}
},
treeForApp() {
return require('path').join(__dirname, 'app-custom');
},
treeForAddon() {
return require('path').join(__dirname, 'addon-custom');
},
`
);
merge(suppressedCustom.files, {
'addon-custom': {
'suppressed-custom': {
'addon-example.js': '// example',
},
},
'app-custom': {
'app-example.js': '// example',
},
});

build = await BuildResult.build(app, { stage: 1, type: 'app' });
expectFile = expectFilesAt(build.outputPath);
});
Expand Down Expand Up @@ -376,5 +428,21 @@ describe('stage1 build', function () {
test('addon with customized ember-addon.main can still use stock trees', function () {
expectFile('node_modules/moved-main/helpers/hello.js').matches(/hello-world/);
});

test('addon with customized treeFor can suppress a stock tree', function () {
expectFile('node_modules/suppressed/_app_/app-example.js').doesNotExist();
});

test('addon with customized treeFor can pass through a stock tree', function () {
expectFile('node_modules/suppressed/addon-example.js').exists();
});

test('addon with customized treeFor can suppress a customized tree', function () {
expectFile('node_modules/suppressed-custom/_app_/app-example.js').doesNotExist();
});

test('addon with customized treeFor can pass through a customized tree', function () {
expectFile('node_modules/suppressed-custom/addon-example.js').exists();
});
});
});