Skip to content

RFC: add missingExport hook - #1845

Merged
lukastaegert merged 1 commit into
rollup:release-0.55.0from
kellyselden:missingExport
Jan 23, 2018
Merged

RFC: add missingExport hook#1845
lukastaegert merged 1 commit into
rollup:release-0.55.0from
kellyselden:missingExport

Conversation

@kellyselden

Copy link
Copy Markdown
Contributor

It would be great to override the behavior of what to do when an export is not found. In most cases, an error is what you want, but in the Ember.js world, leniency would be better.

In Ember.js, ES6 code is transpiled to AMD modules for the browser. This has the side-effect of jamming all exports into a default export. This means that you can write invalid ES6 export/import code that "just works" because of the AMD transpilation.

I would like to gracefully handle this case with a hook. I will then be printing a deprecation warning to Ember users so that invalid Ember code will eventually self-correct,

@kellyselden

Copy link
Copy Markdown
Contributor Author

rebased

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this does not hurt even though I would expect rollup to fail in most cases if an export is missing. Just have a small naming suggestion, otherwise I think this can go into the next release.

Comment thread src/Graph.ts
isPureExternalModule: (id: string) => boolean;
legacy: boolean;
load: (id: string) => Promise<SourceDescription | string | void>;
missingExport: MissingExportHook;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I feel this would look less confusing if this name sounded more like a handler, e.g. onMissingExport or handleMissingExport.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might want to change the name of the default handler as well. For the hook itself, I think the name is good.

Comment thread src/rollup/index.ts
options?: (options: InputOptions) => void;
load?: LoadHook;
resolveId?: ResolveIdHook;
missingExport?: MissingExportHook;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here I think here the name missingExport is ok as it is more of a label.

@lukastaegert
lukastaegert changed the base branch from master to release-0.55.0 January 21, 2018 20:46
@lukastaegert lukastaegert added this to the 0.55.0 milestone Jan 21, 2018
@lukastaegert

Copy link
Copy Markdown
Member

I have changed the base branch to the new release branch, maybe you want to rebase your commit.

@lukastaegert
lukastaegert merged commit de3ba8c into rollup:release-0.55.0 Jan 23, 2018
@kellyselden
kellyselden deleted the missingExport branch January 28, 2018 03:32
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.

2 participants