Skip to content

Conversation

@buschtoens
Copy link
Contributor

As per my message on Discord it seems that native class syntax is actually properly supported by CoreObject and ember-cli.

@buschtoens buschtoens changed the title refactor: convert addon main class to native class syntax refactor: convert addon build code to native class syntax Sep 12, 2019
@buschtoens buschtoens marked this pull request as ready for review September 12, 2019 12:00
}

// https://github.com/ember-cli/ember-cli/blob/cf55d3c36118e6a04ace1cf183951f310cfca9cd/lib/cli/lookup-command.js#L18
CleanCommend.prototype.name = 'ts:clean';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might wanna use a fancy @proto decorator instead. 😄

@dfreeman
Copy link
Member

Thanks for the PR, @buschtoens! So far we've intentionally avoided subclassing entities directly from ember-cli because it can put you in a very weird state when working with linked packages.

If ember-cli-typescript is linked (or is a dependency of a package that's linked) to your main project, then the entities it subclasses would be coming from the version of ember-cli in the linked package while being consumed by the version from the host. There are no guarantees that an Addon instance from one version will work when plugged into another (and I know of some changes over the years that would have caused breakage like that), so while it does work to export an actual class here, you can still wind up with odd problems that are hard to debug.

IIRC I had a conversation with someone about this a while back (maybe @pzuraq?) — what we'd really need for this to be safe would be a way to ensure that the base class we're importing is actually the one for the running CLI instance. I could imagine hacking a best-effort version of that with something like this:

function requireLocal(modulePath) {
  try {
    return require(resolve.sync(modulePath, { basedir: process.cwd() }));
  } catch (e) {
    if (e.code === 'MODULE_NOT_FOUND') {
      return require(modulePath);
    } else {
      throw e;
    }
  }
}

export const Addon = requireLocal('ember-cli/lib/models/addon');

There's also project.require, but there's the Catch-22 of that not being available until after your entity has been instantiated.

Neither option seems super appealing to me, but I imagine community desire for using 'real' syntax for these things is only going to grow (particularly if developing build-time code in TS becomes more common), so maybe there's an approach to solving this upstream.

@buschtoens
Copy link
Contributor Author

Thanks for the detailed explanation! That is indeed a problem and very annoying limitation; not unique to ember-cli though, so there surely must be other projects, which are running into the same issues regarding linked packages. But alas, my Google-Fu is failing me.

I imagine community desire for using 'real' syntax for these things is only going to grow (particularly if developing build-time code in TS becomes more common), so maybe there's an approach to solving this upstream.

Definitely. I feel uncomfortable writing pure JS by now. Unfortunately, the ember-cli-entities workaround comes with its own trade-offs and limitations. :/

@chriskrycho
Copy link
Member

I'd add that I at least want to get the heck away from CoreObject before we start trying to move this way. It's not necessary at this point as far as I can tell; it's just hard to drop because of the ecosystem state.

@chriskrycho
Copy link
Member

@buschtoens any chance you can make the suggested tweaks here?

@buschtoens
Copy link
Contributor Author

buschtoens commented Dec 2, 2019

@chriskrycho Happy to do so, I just wasn't sure whether requireLocal would be a "good enough" solution already for this PR to get merged.

Dropping CoreObject is not a requirement?

@chriskrycho
Copy link
Member

We chatted about this, and we still super appreciate the work (three years later 😂) but are going to close it:

  • We're working on an RFC to switch ember-cli-typescript into maintenance mode—details forthcoming, but the short version is: the broader ecosystem has moved around us to a default of having folks just run tsc directly as part of dev and CI, while using tools like Babel, ESBuild, SWC, etc. for the transpilation, and keeping those fully decoupled. We want to match that (and as folks move into an Embroider-native world, they will be able to opt into things like ts-loader in webpack if they want errors injected into the build).

  • All the points discussed above back when this was opened originally.

Net, we want to minimize further direct investment in ember-cli-typescript internals. Thanks again, @buschtoens!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants