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

V2 addon #397

Merged
merged 7 commits into from
Mar 16, 2022
Merged

V2 addon #397

merged 7 commits into from
Mar 16, 2022

Conversation

SergeAstapov
Copy link
Collaborator

No description provided.

@SergeAstapov SergeAstapov force-pushed the convert-monorepo branch 2 times, most recently from 220f996 to 30ea400 Compare March 2, 2022 05:33
Base automatically changed from convert-monorepo to master March 2, 2022 20:46
@SergeAstapov SergeAstapov force-pushed the v2-addon branch 2 times, most recently from a5fca08 to c8052cf Compare March 2, 2022 20:57
@SergeAstapov SergeAstapov force-pushed the v2-addon branch 2 times, most recently from 8498755 to be8be08 Compare March 11, 2022 03:41
"ember-animated": "0.12.0",
"ember-animated-tools": "github:ember-animation/ember-animated-tools#1edcfd7f0be0a9b4b69450f0fc4d945f31277215",
"ember-animated": "workspace:*",
"ember-animated-tools": "file:../ember-animated-tools-0.5.0.tgz",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As ember-animated-toolsnow a monorepo, we can't use github:ember-animation/ember-animated-tools#v2-addon scheme.

as temp workaround, packed ember-animated-tools and used here.

Once ember-animated-tools published to npm (via beta or actual release), will have a follow up PR to point back to npm.


module.exports = async function () {
return {
usePnpm: true,
scenarios: [
{
name: 'ember-lts-3.12',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

template co-location needs 3.13 (unless I'm missing something)

"ember-auto-import": "^2.4.0",
"ember-cli": "~4.1.1",
"ember-cli-addon-docs": "^4.2.1",
"ember-cli-addon-docs": "file:./ember-cli-addon-docs-4.2.2.tgz",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Couldn't just use branch from ember-learn/ember-cli-addon-docs#1164 as had to remove prepare script otherwise it was trying to link some packages which was failing under ember-animated.

We actually may need to restructure ember-cli-addon-docs into monorepo to get rid from that scripts/link-them.sh but it's another story.


let origNow = clock.now;
let origNow = getOrCreate('time-control', () => clock.now);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ef4 so this is the "trick" I've mentioned during Embroider Office Hour, which is a workaround for embroider-build/ember-auto-import#503

import TimeControl from './time-control';

export { TimeControl };
export { default as MotionTester } from './motion-tester';

// Re-export to ensure "instanceof" works properly within MotionTester.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ef4 and this is second part of the trick to make sure we use same entry-point for the code used in tests that involve instanceof which is a workaround for embroider-build/ember-auto-import#503

@@ -1297,7 +1298,30 @@ class CopiedCSS {
'box-shadow': string;
}

const COPIED_CSS_PROPERTIES = Object.keys(new CopiedCSS());
const COPIED_CSS_PROPERTIES = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The snippet Object.keys(new CopiedCSS()); did not work anymore as TS was removing ass unused class properties defined in CopiedCSS class.

As far as I understand, this is expected TS behavior hence made the list of props separately from the class interface.

// We should remove this file and getOrCreate usage once ember-auto-import gets fixed,
// so we would have single entry point for app and tests.
// Link to track status: https://github.com/ef4/ember-auto-import/issues/503
export function getOrCreate<T>(key: string, construct: () => T): T {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is heart of the "true singleton across webpack entrypoints".
hacky but works 🙃

@@ -38,11 +38,7 @@ let handlerCounter = 0;

let BaseTaskProperty: { new (): HostObject };

if (gte('3.10.0')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is no more support for Ember <3.10

init(...args) {
this._super.init.apply(this, args);

this.treePaths.addon = 'src';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is needed for ember-cli-addon-docs to property find addon code, part of ember-learn/ember-cli-addon-docs#1164

I was thinking we actually should move this to @embroider/addon-shim, @ef4 what do you think?

"@babel/plugin-proposal-decorators", {
"version": "legacy"
}
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had an issue with @babel/plugin-proposal-class-properties and decided to remove it.

I had a problem with decorated class properties.
e.g. @service('-ea-motion') motionService!: MotionService; was properly transpiled to decorate class property, but in addition I got this.motionService = null added to the constructor which cleaned up property value.

I assume class properties actually would be processed by babel in the consuming app at build time.

e.g. if app supports only latest browser with class properties support, having @babel/plugin-proposal-class-properties plugin in the addon would still transpile it to ES6 which does not seem to me as ideal behavior.

I'm 100% missing something here but not sure what.

@SergeAstapov SergeAstapov marked this pull request as ready for review March 11, 2022 04:32
@SergeAstapov SergeAstapov requested a review from ef4 March 11, 2022 04:32
Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

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

Looking great, thanks for all your hard work.

I left one question but that's not a blocker.

I'm fine with the workarounds for the module duplication problem. In general it's healthy for NPM packages to be tolerant of duplication, even if in this case the duplication is a bug.

// These are the modules that users should be able to import from your
// addon. Anything not listed here may get optimized away.
addon.publicEntrypoints([
'-private/**/*.{js,ts}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this? I would have thought nothing in the app should be trying to import from these paths.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ef4 there are bunch of unit tests importing from -private folder, e.g. https://github.com/ember-animation/ember-animated/blob/master/test-app/tests/unit/adjust-color-test.js

I'm not sure there is way to make it without having -private in publicEntrypoints

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see.

We will have to think about whether there's a nice way for v2 addons to have a test build that offers entrypoints to the test app that aren't part of the published build (without risking that tests become unrealistic).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants