Migrate to a v2 addon

[jelhan]

v2 addons are the feature. We should migrate to it. Not only but also to improve build time for the users of Ember Bootstrap.

Likely that will be also the point in time at which we switch to a real monorepo. See #1304.

[jelhan]

Migrating to a v2 addon may not be straightforward. We are using some pattern in Ember Bootstrap, which I'm unsure if they are supported by v2 addons:

• Injecting a HTML element in index.html using contentFor hook:

  ember-bootstrap/index.js

  Lines 230 to 238 in 87a01a2

  	contentFor(type, config) {
  	if (
  	type === 'body-footer' &&
  	config.environment !== 'test' &&
  	this.bootstrapOptions.insertEmberWormholeElementToDom !== false
  	) {
  	return '<div id="ember-bootstrap-wormhole"></div>';
  	}
  	},

• Injecting Ember Bootstrap specific CSS into the bundle:

  ember-bootstrap/index.js

  Line 110 in 87a01a2

  this.import(path.join(vendorPath, `bs${options.bootstrapVersion}.css`));

• Injecting Bootstrap CSS in the bundle if configured to do so:

  ember-bootstrap/index.js

  Lines 90 to 107 in 87a01a2

  	if (!this.hasPreprocessor()) {
  	// / Import css from bootstrap
  	if (options.importBootstrapCSS) {
  	this.needsBootstrapStyles = true;
  	this.import(path.join(vendorPath, 'bootstrap.css'));
  	this.import(path.join(vendorPath, 'bootstrap.css.map'), {
  	destDir: 'assets',
  	});
  	}
  	if (options.importBootstrapTheme) {
  	this.needsBootstrapStyles = true;
  	this.import(path.join(vendorPath, 'bootstrap-theme.css'));
  	this.import(path.join(vendorPath, 'bootstrap-theme.css.map'), {
  	destDir: 'assets',
  	});
  	}
  	}

• Making Bootstrap SASS files available to consuming applications by pushing them into the build:

  ember-bootstrap/index.js

  Lines 180 to 186 in 87a01a2

  	treeForStyles() {
  	if (this.hasPreprocessor()) {
  	return new Funnel(this.getBootstrapStylesPath(), {
  	destDir: 'ember-bootstrap',
  	});
  	}
  	},

• Excluding components from being pushed in the bundle using include / exclude configuration options:

  ember-bootstrap/index.js

  Lines 205 to 208 in 87a01a2

  	treeForApp(tree) {
  	tree = this.filterComponents(tree);
  	return this._super.treeForApp.call(this, tree);
  	},

We need to verify if all of this is possible with v2 addons. I doubt so.

If it's not supported, we need to define path forward for each of those features. Having both Embroider and classic apps in mind.

[simonihmig]

Injecting a HTML element in index.html using contentFor hook:

Yeah, we cannot do this with v2, but also this should be easy to provide other means. Thinking aloud, we could auto-create that div as a direct child of the app's rootElement if it doesn't exist maybe? And/or tell users to add it to their index.html or application.hbs or wherever they want it to be.

Injecting Ember Bootstrap specific CSS into the bundle

That should be easily possible (if still needed): https://github.com/embroider-build/embroider/blob/main/docs/v2-faq.md#how-can-i-ship-css-with-my-addon

Injecting Bootstrap CSS in the bundle

I think with v2 addon's pull-based approach, that should become an application concern. We need to tell users that they need to import bootstrap's CSS from e.g. their app.js file. With ember-auto-import that's working ootb, and should also support correct node module resolution, so something like import from 'bootstrap/bootstrap.css';.

Making Bootstrap SASS files available to consuming applications by pushing them into the build:

That's not really pushing into the build, we just make bootstrap's sass files available under our own namespace, so users would not have to @import ../node_module/bootstrap/path/to/bootstrapp.scss. But that should also become an app concern IMO. And the explicit path is not that bad, if still needed. Also mind that Embroider users will want to stop using ember-cli-sass and use a webpack sass plugin for native webpack handling.

Excluding components from being pushed in the bundle using include / exclude configuration options:

We don't need this feature at all anymore with v2. Instead of this "poor-man's" tree-shaking users (of Embroider or classic + ember-auto-import) will get real tree-shaking ootb. One thing less to worry!

So tl;dr I think there is nothing stopping us from adopting the v2 format, other than our time 😅. Some changes will have to become breaking ones, but that should be fine. The world is moving on... :)

[jelhan]

Excluding components from being pushed in the bundle using include / exclude configuration options:

We don't need this feature at all anymore with v2. Instead of this "poor-man's" tree-shaking users (of Embroider or classic + ember-auto-import) will get real tree-shaking ootb. One thing less to worry!

Do we get tree-shaking for components out of the box? I thought tree-shaking components requires an application to 1) use Embroider and 2) set staticComponents to true explicitly in Embroider configuration.

[simonihmig]

yeah, you are right about that!

Technically, you could also get tree-shaking in classic apps when not relying on app-reexports. The latter is what makes components get pushed into the bundle eagerly. While this is a direction (getting rid of app-reexports) we will hopefully get to eventually with template-tag and explicit imports everywhere, we are not there yet (for the average user). So yes, for classic build users this will be some kind of step backwards.

But I still think this is ok, as when people care about bundle size, they should work towards getting to Embroider and all static flags enabled. We shouldn't try to solve the same problem in different ways and in different places, like providing different "tree-shaking" mechanisms for each addon. IMHO.

[jelhan]

So yes, for classic build users this will be some kind of step backwards.

But I still think this is ok, as when people care about bundle size, they should work towards getting to Embroider and all static flags enabled. We shouldn't try to solve the same problem in different ways and in different places, like providing different "tree-shaking" mechanisms for each addon. IMHO.

While I fully agree with you in theory, in reality the apps I care about, which are using Ember Bootstrap, are not migrated to Embroider yet. But they are using the include / exclude feature of Ember Bootstrap.

I tend towards keep support for "poor-mans tree-shaking" via include / exclude at least until newly generated Ember apps support tree-shaking out of the box.

[SanderKnauff]

I've been attempting to port the addon over to V2 and I've encountered the following findings:

• The include/exclude options are impossible to implement in v2 addons due to all treeFor build hooks being dropped [1]. I wonder if there are any other options to manually remove content from the build. If there is, we could instruct users to use those.
• Injecting ember-bootstrap-wormhole should still be possible according to the spec [2], I have not found a way however to make that work. I have tested a workaround and that seems to be okay, but I still wonder how to use some of the new package features.
• Macro helpers seem to be broken at the moment. For some reason, only the JS part of @embroider/macros is being converted. I have opened a ticket in the embroider repo [3], but I'm still waiting for a response. It was in the middle of the holidays when I filed it, so I'm not too surprised if it takes some time.
• Speaking of macros, I'm still trying to figure out how to provide the default config. Previously this was set in index.js, so I wonder if we can inject it somewhere in the addon-main.js. I have not found many examples of v2 addons using macros yet, and if they do, they don't seem to be setting default values.

My current working branch can be found at https://github.com/SanderKnauff/ember-bootstrap/tree/v2-addon.

[1] https://github.com/embroider-build/embroider/blob/main/docs/spec.md#package-hooks
[2] https://github.com/embroider-build/embroider/blob/main/docs/spec.md#build-time-conditionals
[3] embroider-build/embroider#1745

[jelhan]

• The include/exclude options are impossible to implement in v2 addons due to all treeFor build hooks being dropped [1]. I wonder if there are any other options to manually remove content from the build. If there is, we could instruct users to use those.

I opened an issue in Embroider repository to investigate if such a support could be added: embroider-build/embroider#1748

I also experimented using macroCondition from @embroider/macros to remove entire components from the build depending on build-time configuration:

import { getOwnConfig, macroCondition } from '@embroider/macros';

let defaultExport;

if (macroCondition(getOwnConfig().includeConditionalComponent)) {
  defaultExport = <template>
    <p>
      Hello, conditional component!
    </p>
  </template>
}

export default defaultExport;

It seems to work for the simple cases, which I have tested. But only if using template tags. Otherwise the build throws when trying to associate the template with component's JavaScript class, which does not exist. I'm also not sure if that's a good way forward. I don't think doing something like that is officially supported. And I fear we may run into unexpected edge cases and bugs with different versions of Ember, Embroider, and all other upstream tools involved.

[jelhan]

Speaking of macros, I'm still trying to figure out how to provide the default config. Previously this was set in index.js, so I wonder if we can inject it somewhere in the addon-main.js. I have not found many examples of v2 addons using macros yet, and if they do, they don't seem to be setting default values.

I fear this is not implemented yet: embroider-build/embroider#1303

[ef4]

I think if you try to do a one-to-one port you're going to have a hard time. But that's not what I think you should do.

I think Ember Bootstrap should be only a component library of ember-specific bootstrap-using components. Everything else is not an addon concern.

For example: today ember-bootstrap needs an option for "importBootstrapTheme". But that's an app concern, it should be the app's decision to import a bootstrap theme or not.

Back in the days when the only way to integrate a complex library like bootstrap into an Ember app was to write gnarly broccoli code, it was good to have addons like ember-bootstrap to handle that complexity. But those days are long past, and every ember app can directly import from bootstrap and follow bootstrap's own documentation on how to set it up. The remaining valuable thing about ember-bootstrap is the components.

If you make bootstrap a peerDependency you can use dependencySatisfies to do tree-shaking against the bootstrap version with no custom configuration.

[jelhan]

@ef4: Thanks a lot for sharing your thoughts! Highly appreciated.

I think Ember Bootstrap should be only a component library of ember-specific bootstrap-using components. Everything else is not an addon concern.

I think we all agree that Ember Bootstrap should drop support for importing Bootstrap CSS, theme, setting up SASS, etc. There seems to be clear path forward and beside a little churn no regression for consumers.

But there are 2 items, which would be a regression for some consumers of Ember Bootstrap:

1. We cannot support "poor-man's" tree-shaking through include / exclude options anymore. Apps relying on that feature would need holding back the upgrade until finalizing migration to Embroider with optimized configuration.
2. Relying on dependencySatisifies for detecting used Bootstrap version without configuration fallback forces a dependency on consumers, even though they may not need it.

It may be surprising that some consumers of Ember Bootstrap may not need a dependency on bootstrap at all. The reason is the following:

1. Ember Bootstrap does not depend on bootstrap NPM package itself.
2. Consumers may use Bootstrap CSS with a theme already being applied. Either provided as CSS by another NPM package or hosted on a CDN.

As said it's not a clear blocker. But causing a trade-off decision. Mainly if the build-time improvements consumers get from v2 addon outweigh the two trade-offs listed above.

[SanderKnauff]

If deprecating these features is a concern, I think it's best to change up the planning into the following:

1. Convert the addon into a monorepo (Consider switching to a real monorepo #1304). This is a step we have to take eventually when going to v2, and most of the work has (I think) already been done. I have to check if my current commits are functional for that.
2. Add Typescript support for the current components (Road to TypeScript #2053). This has the downside of having to redo the Typescript configuration for the addon code itself, but will give anyone 'stuck' on the pre-v2 version access to the benefits of Typescript and Glint.
3. (and maybe in between 1 and 2) Deprecate the build-time logic and tree shaking capabilities.
4. Convert to a V2 addon (Migrate to a v2 addon #1980).
5. GTS/GJS? This could be done together with Typescript support, but I'm not to sure how breaking this will be.

With this we look at the following versions:

7.x with Typescript support and build-time deprecations
8.x with V2 support.

Would this be an acceptable plan?

[simonihmig]

The include/exclude options are impossible to implement in v2 addons

As Ed laid out in embroider-build/embroider#1748, I think the approach to extend the treeForAddon/App hooks and apply basically the same funneling logic that we already have seems like the way forward for me. I believe typical e-b users might not be that far ahead of the wave to already have adopted template tag (which would allow us to drop app-reexports). And limiting our "poor-man's" tree-shaking to classic builds only is good enough I think for existing users, isn't it?

Injecting ember-bootstrap-wormhole should still be possible according to the spec

The spec you linked to is not the one from the official RFC, but even that hasn't been fully implemented. Specifically those build hooks are not implemented, and AFAIK deliberately so!

Relying on dependencySatisifies for detecting used Bootstrap version without configuration fallback forces a dependency on consumers, even though they may not need it.

Yeah, and given that we had this as an explicit config option, I would tend to keep it as such...

Speaking of macros, I'm still trying to figure out how to provide the default config.

Coincidentally, I played around just recently with an approach for v2 addon configs, trying to give users a simpler way to setup the configuration, which the addon applies as macro configs: https://github.com/simonihmig/ember-addon-config/tree/main. I wanted to get feedback from the Embroider team on what they think about this, so it's still more of a spike than something I would want people to widely adopt already...

Either way, keeping our bootstrapVersion option seems fine for me. The other options related to importing styles will go away though, I think we agree on that...

7.x with Typescript support and build-time deprecations
8.x with V2 support.

With my take on tree-shaking as stated above, I think we can aim for v2 migration for v7. Migrating to TS is then just a non-breaking feature we can add as a minor version during the 7.x cycle. What do you think?

[jelhan]

I created two issues for deprecating features, which we agreed to drop:

• Deprecate Bootstrap CSS, fonts, and theme import #2063
• Deprecate inserting wormhole target #2064

I feel deprecating the features we agreed on dropping, will help us breaking the migration to v2 addon in smaller steps. It also gives consumers some more time migrating to recommended patterns and avoiding many unexpected breaking changes when we ship as a v2 addon in one of the next major releases.

Deprecating exporting Bootstrap's SASS under Ember Bootstrap's namespace isn't straight forward. We cannot detect if consumers actually use it. Currently it cannot be even turned off by consumers. I would tend towards not deprecating but just removing it.