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

fix(start-module): require plugins instead of import #651

Closed
wants to merge 1 commit into from

Conversation

Weakky
Copy link
Collaborator

@Weakky Weakky commented Apr 13, 2020

Description

When using import, ts.transpileModule removes the import if the import is not used.

As the plugin imports are only there to enable tree-shaking (but the plugins aren't used), the imports were removed.

This PR uses require instead, which prevents the imports from being removed

@Weakky Weakky requested a review from jasonkuhrt April 13, 2020 17:31
@jasonkuhrt
Copy link
Member

jasonkuhrt commented Apr 13, 2020

But tree-shaking does not work with require. There must be another way.

I don't think there's a problem here. When we get to it, we will support a build where ES Modules are left in tact.

From there, what TS has produced, will get handed off to something like parcel.js.

@Weakky
Copy link
Collaborator Author

Weakky commented Apr 13, 2020

But tree-shaking does not work with require.

Ohh, ofc, makes sense 👍

I don't think there's a problem here

Well, the problem right now is that all plugin imports are trimmed out. So if one tries to tree-shake the app, I assume the runtime plugins wouldn't be part of the bundle

@jasonkuhrt
Copy link
Member

So if one tries to tree-shake the app, I assume the runtime plugin wouldn't be part of the bundle

Correct, tree-shaking will not work as is. And probably we want to build it in but let it be disabled, overriden, etc.

For example user should be able to adjust their tsconfig and make TS output imports instead of requires.

Don't think we have an issue for this yet, will create it.

@jasonkuhrt
Copy link
Member

Actually we do graphql-nexus/nexus#119

@jasonkuhrt
Copy link
Member

@Weakky Anything left to discuss or shall we close?

@Weakky
Copy link
Collaborator Author

Weakky commented Apr 14, 2020

Let's close 👍

@Weakky Weakky closed this Apr 14, 2020
@jasonkuhrt jasonkuhrt deleted the fix/plugin-require branch April 14, 2020 12:13
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