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

Conflict between JS module API and Module Unification #16361

Closed
tomdale opened this issue Mar 10, 2018 · 3 comments
Closed

Conflict between JS module API and Module Unification #16361

tomdale opened this issue Mar 10, 2018 · 3 comments

Comments

@tomdale
Copy link
Member

tomdale commented Mar 10, 2018

I discovered an issue while pairing with @wycats that I wanted to write down for future discussion.

We were upgrading an Ember app to use Module Unification that had already adopted the JS module API. The migrator produced invalid code for helpers due to the following conflict:

  1. Helper functions must be wrapped with the helper function exported by @ember/component/helper.
  2. In Module Unification, helpers must be a named export called helper.

The migrator produces code that looks like this:

import { helper } from '@ember/components/helper';
function myHelper() { /* ... */ }
// Error: cannot redeclare `helper`
export const helper = helper(myHelper);

One way to fix this is for the migrator to detect this conflict and rename the import (since the export name cannot be changed), e.g. import { helper as emberHelper } from '@ember/components/helper'. But this is something that everyone will have to type, so we may want to think about this conflict and if there are design changes we want to consider.

@NLincoln
Copy link

One solution would be to rename helper to createHelper

So the above example would become:

import { createHelper } from '@ember/components/helper';
function myHelper() {}
export const helper = createHelper(myHelper);

This can be done in a backwards compatible way by exporting both helper and createHelper from @ember/components/helper, and by mapping both to Ember.Helper.helper in the mappings.

@mixonic
Copy link
Sponsor Member

mixonic commented Mar 13, 2018

@NLincoln yeah that is a fine approach. We will probably do it in the migrator though:

import { helper as buildHelper } from '@ember/components/helper';
function myHelper() {}
export const helper = buildHelper(myHelper);

This work should be done in https://github.com/rwjblue/ember-module-migrator

/cc @rwjblue @bantic

@bantic
Copy link
Member

bantic commented Mar 13, 2018

A fix for this is released in the module migrator in v0.8.0

@rwjblue rwjblue closed this as completed Mar 23, 2018
bantic added a commit to bantic/ember-rfc176-data that referenced this issue Apr 2, 2018
This change allows the ember-modules-codemod to generate transformed helper files
that `import { helper as buildHelper }`. This brings it
into alignment with the ember-module-migrator's output.

The related change in the module migrator is here: ember-codemods/ember-module-migrator#69

Importing the helper as buildHelper allows the module migrator to `export const helper = buildHelper(...`,
which is necessary to avoid an issue where incorrect `export const helper = helper(...` code was being generated.

The issue that motived the helper->buildHelper change is: emberjs/ember.js#16361 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants