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

remove private api from MissingExport hook #1987

Merged
merged 1 commit into from
Mar 12, 2018

Conversation

kellyselden
Copy link
Contributor

Closes #1973.

I don't think calling Module functions is going to be necessary after all.

src/Graph.ts Outdated
@@ -63,7 +62,7 @@ export default class Graph {
isPureExternalModule: (id: string) => boolean;
legacy: boolean;
load: (id: string) => Promise<SourceDescription | string | void>;
handleMissingExport: MissingExportHook;
handleMissingExport: (module: Module, name: string, otherModule: string, start?: number) => void;
Copy link
Contributor

@guybedford guybedford Feb 19, 2018

Choose a reason for hiding this comment

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

I take it module is the importer here, and start is the index in the importer? How about exportName, importingModule, importedModule, importerStart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the wording and order.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Much better, would be great to get this in.

I'd still be interested to hear more background on the use cases here.

@kellyselden
Copy link
Contributor Author

https://github.com/jmurphyau/ember-truth-helpers/blob/v2.0.0/app/helpers/not-eq.js#L1
reexports notEq
https://github.com/jmurphyau/ember-truth-helpers/blob/v2.0.0/addon/helpers/not-equal.js#L3
notEq doesn't exist, only notEqualHelper is exported.
It was probably a refactor gone wrong. But when compiled down to AMD modules for the browser, the code doesn't throw because everything is essentially properties on a "global" export. If you were to try to call the broken code, it would throw, but if you don't use it, the app works fine.

define('ember-truth-helpers/helpers/not-equal', ['exports'], function (exports) {
  'use strict';

  Object.defineProperty(exports, "__esModule", {
    value: true
  });
  exports.notEqualHelper = notEqualHelper;
  function notEqualHelper(params) {
    return params[0] !== params[1];
  }

  exports.default = Ember.Helper.helper(notEqualHelper);
});
define('my-app/helpers/not-eq', ['exports', 'ember-truth-helpers/helpers/not-equal'], function (exports, _notEqual) {
  'use strict';

  Object.defineProperty(exports, "__esModule", {
    value: true
  });
  Object.defineProperty(exports, 'default', {
    enumerable: true,
    get: function () {
      return _notEqual.default;
    }
  });
  Object.defineProperty(exports, 'notEq', {
    enumerable: true,
    get: function () {
      return _notEqual.notEq;
    }
  });
});

This is why I need a missing export hook, and a way to continue the graph traversal for backwards compatibility. The hook allows me to return true so rollup can continue without stopping, and a way to print a deprecation message to Ember users. Does that make sense?

@guybedford
Copy link
Contributor

Completely, thanks for explaining it to me here! The thing to note is that this won't ever support the es module output and will likely throw an internal error or have other undefined behaviour if this early error is skipped.

So ideally we should actually remove the missing export from the analysis entirely so that tracing can go on without it I think.

@guybedford
Copy link
Contributor

Actually thinking through the cases here I believe this will output correctly even for es modules with code splitting, as we only create the import/exports where tracing succeeds.

So this looks good to merge here to me.

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

3 participants