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

Initial implementation of co-located templates RFC. #249

Merged
merged 34 commits into from Sep 24, 2019
Merged

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 30, 2019

Still quite WIP, but "basically works" when using ember-cli 3.11 and Ember latest canary (including changes from emberjs/ember.js#18158.

ember-addon-main.js Outdated Show resolved Hide resolved
Still quite WIP, but "basically works" when using ember-cli master which includes

ember-cli/ember-cli@84c449a

Co-authored-by: Robert Jackson <me@rwjblue.com>
Copy link

@devotox devotox left a comment

Choose a reason for hiding this comment

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

This breaks this addon https://github.com/sethwebster/ember-cli-new-version/tree/master/addon/components/new-version-notifier with this error

Error: Could not find module ember-cli-new-version/components/new-version-notifier/template imported from ember-cli-new-version/components/new-version-notifier/component

@chriskrycho
Copy link
Contributor

This is currently breaking with the @classic decorator invocation because of how it's transforming the component declaration.

SyntaxError: /Users/chris/dev/test/new-stuff-yo/new-stuff-yo/components/lame.js: Leading decorators must be attached to a class declaration (6:0)

  4 | 
  5 | @classic
> 6 | const CLASS = class Lame extends Component {
    | ^
  7 | }
  8 | 
  9 | const setComponentTemplate = Ember._setComponentTemplate;

This isn't a problem with the defaults, since the default behavior is to generate a .extend({ ... })-style class, but for anyone migrating via @classic and using colocation, it's a breaker, and one without a workaround. (For folks looking on: the transform as written actually matches the ES spec, but the Stage 1 transforms and any TS-dependent editor tooling, including VS Code's, falls down on the revised spec; and all existing docs I'm aware of specify the Stage 1 export position.)

We can now rely on Node 8+, so this is "safe".
Running `BROCCOLI_DEBUG=ember-cli-htmlbars:* ember b` will now emit
files into `DEBUG/ember-cli-htmlbars/` showing the various stages of
compilation (input, after colocation processing, and final output).
@@ -1,6 +1,6 @@
{
"name": "ember-cli-htmlbars",
"version": "3.1.0",
"version": "3.2.0-alpha.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll need to tweak this, as there are breaking changes now on master.

package.json Outdated Show resolved Hide resolved
@rwjblue
Copy link
Member Author

rwjblue commented Sep 5, 2019

@chriskrycho - Can you share a snippet of what you started with? The error doesn't make it clear...

@chriskrycho
Copy link
Contributor

Yes, of course! Original input:

import Component from '@ember/component';
import classic from 'ember-classic-decorator';

@classic
export default class Lame extends Component {}

@pzuraq
Copy link

pzuraq commented Sep 5, 2019

Got an issue popping up over in ember-set-helper: adopted-ember-addons/ember-set-helper#6

Seems like a custom transform is not being run against colocated templates. I copied the template in the reproduction app into the app/templates/components directory and things started working, so it seems to be something with ember-cli and colocation specifically.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 14, 2019

OK, landed the changes from #280 into here and that should have addressed quite a few of the issues folks have reported:

  • @chriskrycho's comment RE: using decorators before the export
  • @pzuraq's comment about preprocessors not running properly (we now do exactly the same as hbs, and that works well with custom transforms)

@rwjblue rwjblue marked this pull request as ready for review September 15, 2019 17:35
@rwjblue rwjblue changed the title Initial spike of co-located templates RFC. Initial implementation of co-located templates RFC. Sep 23, 2019
@rwjblue rwjblue merged commit 6acc791 into master Sep 24, 2019
@rwjblue rwjblue deleted the colocation branch September 24, 2019 00:05
@rwjblue rwjblue restored the colocation branch September 24, 2019 00:05
@rwjblue rwjblue deleted the colocation branch August 10, 2020 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants