Remove test-specific behaviour for BasicDropdownWormhole

[rohitpaulk]

Fixes #931.

I tried reading through the history of this to figure out why it was present, but nothing turned up. The commit that added the test-specific condition was 067ca88.

cc: @mkszepp in case you've got more context on why this was needed?

Reasoning behind removing this: If tests are using application.hbs, the component should work as-is. If tests aren't using application.hbs (component tests for example), the wormhole will need to be setup manually anyway + it'll still need to have the wormhole-specific ID, not ember-testing.

[mkszepp]

@rohitpaulk the code was added in this commit PR #743 ... i have only synced the code parts like it is in basic-dropdown, no specific condition.

ember-basic-dropdown/ember-basic-dropdown/src/components/basic-dropdown.ts

Lines 385 to 413 in b078018

	_getDestinationId(): string {
	// eslint-disable-next-line @typescript-eslint/ban-ts-comment
	// @ts-ignore
	const config = getOwner(this).resolveRegistration('config:environment') as {
	environment: string;
	APP: {
	rootElement: string;
	};
	'ember-basic-dropdown': {
	destination: string;
	};
	};
	if (config.environment === 'test') {
	// document doesn't exists in fastboot apps, for this reason we need this check
	if (typeof document === 'undefined') {
	return 'ember-basic-dropdown-wormhole';
	}
	const rootElement = config['APP']?.rootElement;
	return (
	document.querySelector(rootElement)?.id ??
	'ember-basic-dropdown-wormhole'
	);
	}
	return ((config['ember-basic-dropdown'] &&
	config['ember-basic-dropdown'].destination) ||
	'ember-basic-dropdown-wormhole') as string;
	}

Can you add any test that we can see that it works fine also after this changes?

[rohitpaulk]

@mkszepp thanks for the context! I've added tests for this.

The commit you linked to indeed only re-arranges stuff and doesn't alter logic. 067ca88 seems to be where the isTesting condition was added. That's the one I wasn't sure about - do you remember if there was a specific reason to add the isTesting check?

[rohitpaulk]

Looks like there a shadow dom test failure, will check shortly!

[mkszepp]

@rohitpaulk i have checked why the test fails and its not directly caused with you changes... its because we are adding for shadow dom tests already with a initializer the element (for tests unfortunately there is no better solution)

replacing your test dom('#custom-wormhole-destination') with dom('.ember-application #custom-wormhole-destination') solves the issue and test is also correct

ember-basic-dropdown/test-app/app/instance-initializers/shadow-root.js

Lines 22 to 32 in d816d19

	const rootElement = document.createElement('div');
	const wormhole = document.createElement('div');
	wormhole.id = 'ember-basic-dropdown-wormhole';
	hostElement.shadowRoot.appendChild(wormhole);
	hostElement.shadowRoot.appendChild(rootElement);
	targetElement.appendChild(hostElement);
	config.APP.rootElement = '#ember-basic-dropdown-wormhole';
	appInstance.set('rootElement', rootElement);
	}

Unfortunately I have no rights to work on your branch, so I had to make a fork

Thanks for fixing the issue