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

add define.exports #121

Merged
merged 2 commits into from
Jun 15, 2017
Merged

add define.exports #121

merged 2 commits into from
Jun 15, 2017

Conversation

stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented May 25, 2017

  • backfill some readme stuff

Opens the door to (template compiler and ember/glimmer need to learn about this next):

define.exports('my-app/templates/my-template.hbs', { hbs: true, "id": "VVZNWoRm", "block": "{\"statements\":[[1,[26,[\"welcome-page\"]],false],[0,\"\\n\"],[0,\"\\n\"],[1,[26,[\"outlet\"]],false]],\"locals\":[],\"named\":[],\"yields\":[],\"hasPartials\":false}", "meta": { "moduleName": "my-foo-app/templates/application.hbs" }});

instead of:

define("my-foo-app/templates/application", ["exports"], function (exports) {
  "use strict";

  Object.defineProperty(exports, "__esModule", {
    value: true
  });

  exports.default = Ember.HTMLBars.template({ "id": "VVZNWoRm", "block": "{\"statements\":[[1,[26,[\"welcome-page\"]],false],[0,\"\\n\"],[0,\"\\n\"],[1,[26,[\"outlet\"]],false]],\"locals\":[],\"named\":[],\"yields\":[],\"hasPartials\":false}", "meta": { "moduleName": "my-foo-app/templates/application.hbs" } });
});

benefits:

  • no reification step
  • no closure
  • no juggling pre-parse semantics:

TODO:

  • run tests on a larger code-base to get realistic #'s

@stefanpenner stefanpenner requested review from kratiahuja, krisselden and dgeb and removed request for kratiahuja May 25, 2017 15:13
@rwjblue
Copy link
Member

rwjblue commented May 25, 2017

Where does Ember.HTMLBars.template( bit go in the new semantics?

@stefanpenner
Copy link
Contributor Author

Where does Ember.HTMLBars.template( bit go in the new semantics?

I would move it to the caller, which would detect require(templatePath).default.hbs === true or something.

* backfill some readme stuff
@stefanpenner
Copy link
Contributor Author

stefanpenner commented May 25, 2017

Some numbers (note the benchmarks only test runtime cost define + require, likely more tests are required re: eager vs lazy parse stuff and larger example \w many templates so we can see how gzip likes this)

Benchmark
  file: ./benchmarks/scenarios/define-many.js
  total:  73ms
  per op:  7.3ms

Benchmark
  file: ./benchmarks/scenarios/define-many-exports.js
  total:  47ms
  per op:  4.7ms

File size for a single small template (post gzip).

 t/size cat before.js | gzip -9 | wc -c
     318

 t/size cat after.js | gzip -9 | wc -c
     223
// before.js
define("my-foo-app/templates/application", ["exports"], function (exports) {
  "use strict";

  Object.defineProperty(exports, "__esModule", {
    value: true
  });

  exports.default = Ember.HTMLBars.template({ "id": "VVZNWoRm", "block": "{\"statements\":[[1,[26,[\"welcome-page\"]],false],[0,\"\\n\"],[0,\"\\n\"],[1,[26,[\"outlet\"]],false]],\"locals\":[],\"named\":[],\"yields\":[],\"hasPartials\":false}", "meta": { "moduleName": "my-foo-app/templates/application.hbs" } });
});
// after.js
define.exports('my-app/templates/my-template.hbs', { hbs: true, "id": "VVZNWoRm", "block": "{\"statements\":[[1,[26,[\"welcome-page\"]],false],[0,\"\\n\"],[0,\"\\n\"],[1,[26,[\"outlet\"]],false]],\"locals\":[],\"named\":[],\"yields\":[],\"hasPartials\":false}", "meta": { "moduleName": "my-foo-app/templates/application.hbs" }});

@rwjblue
Copy link
Member

rwjblue commented May 25, 2017

I would move it to the caller, which would detect require(templatePath).default.hbs === true or something.

Maybe, but if we do this, then it needs to include version info or something. Should be easy to put into the meta though...

@stefanpenner
Copy link
Contributor Author

stefanpenner commented May 25, 2017

Maybe, but if we do this, then it needs to include version info or something. Should be easy to put into the meta though...

totally agree.

I'll run more thorough tests to confirm/deny my intuition (before we proceed with the required plumbing)

@rwjblue
Copy link
Member

rwjblue commented May 25, 2017

Another issue that we will have to address is how the templates get instantiated. In older Ember versions, templates were not instantiated but as of Ember 2.10 they are instantiated (and receive injections). Today, Ember.HTMLBars.template returns an object that adheres to the standard DI system contract (and we create instances of the templates by calling .create(injectionsHere) on them).

We could swap lookup to use factoryFor().class instead, but we just need to think it through...

@stefanpenner
Copy link
Contributor Author

Today, Ember.HTMLBars.template returns an object that adheres to the standard DI system contract (and we create instances of the templates by calling .create(injectionsHere) on them).

Ya, I think we can wrap on the caller side for this as well.

Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

I like this!

README.md Outdated
* no reification step
* no need to juggle pre-parse voodoo.

### `require.unsee('foo');
Copy link
Member

Choose a reason for hiding this comment

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

Missing a closing `

README.md Outdated

### `require.unsee('foo');

`require.unsee` allows one to unload a given module. *note* The side-affects of that module cannot be unloaded.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should be side-effects

@kratiahuja
Copy link
Collaborator

+1 to this.

Copy link
Contributor

@krisselden krisselden left a comment

Choose a reason for hiding this comment

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

I'm in favor but this is going to require opt in from the caller side, one thing, the meta is extensible on the compilation side and present on the factory as well, so in the old and new world you can check the meta on the export.

@stefanpenner stefanpenner merged commit f62bc54 into master Jun 15, 2017
@stefanpenner stefanpenner deleted the define-exports branch June 15, 2017 20:51
@stefanpenner
Copy link
Contributor Author

released as v4.5.0 🎉

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.

None yet

5 participants