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

refactor TypeScript typings to use ES style exports #5016

Closed
wants to merge 1 commit into from

Conversation

yyx990803
Copy link
Member

@yyx990803 yyx990803 commented Feb 26, 2017

This PR will change Vue's TS typings to use ES-style exports so that it works better with the new ES module build + webpack 2 combo.

So instead of:

import Vue = require('vue')

TS users should now use:

import Vue from 'vue'

...which is consistent with recommended plain ES usage.

This is technically a breaking change, but since we've already unintentionally caused a breaking change for webpack 2 + TS users, we should probably just fully migrate to ES-based exports to avoid more breakage in the future.

Related:

@codecov-io
Copy link

codecov-io commented Feb 26, 2017

Codecov Report

Merging #5016 into dev will increase coverage by 0.69%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             dev   #5016      +/-   ##
========================================
+ Coverage   99.3%    100%   +0.69%     
========================================
  Files         92      86       -6     
  Lines       3737    3488     -249     
========================================
- Hits        3711    3488     -223     
+ Misses        26       0      -26
Impacted Files Coverage Δ
src/compiler/codegen/index.js 100% <0%> (ø) ⬆️
src/platforms/web/compiler/index.js 100% <0%> (ø) ⬆️
src/core/global-api/index.js 100% <0%> (ø) ⬆️
src/core/vdom/create-component.js 100% <0%> (ø) ⬆️
src/core/vdom/create-element.js 100% <0%> (ø) ⬆️
src/platforms/web/runtime/modules/attrs.js 100% <0%> (ø) ⬆️
src/core/global-api/assets.js 100% <0%> (ø) ⬆️
src/compiler/index.js 100% <0%> (ø) ⬆️
src/core/util/options.js 100% <0%> (ø) ⬆️
src/core/instance/render-helpers/resolve-slots.js 100% <0%> (ø) ⬆️
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3a1522...5b5a88f. Read the comment docs.

@yyx990803
Copy link
Member Author

@ktsn @HerringtonDarkholme this will likely also affect vue-class-component and av-ts, so I'd like to get some feedback on whether this will cause potential issues.

@HerringtonDarkholme
Copy link
Member

I need some local test here. Please kindly wait a moment. I guess this typing change will be fine for webpack users.
But I'm not sure about commonjs users.

@ktsn
Copy link
Member

ktsn commented Feb 27, 2017

It seems to break some TypeScript libraries that directly depends on Vue. They need to be updated and also the updated library does not seem to be compatible with old (commonjs style) Vue. It's because TypeScript does not interop ESModule and CommonJS at all (as same as export).

// commonjs style import
import Vue = require('vue')

// will be compiled to ...
var Vue = require('vue')
// esmodule style import
import Vue from 'vue'

// will be compiled to ...
var Vue = require('vue').default

@HerringtonDarkholme
Copy link
Member

@ktsn I think we will need to break something for consistent import style across TS/JS.

But we should consider what part should we break. Or what should we support.

@DanielRosenwasser
Copy link

What exactly is the runtime semantics? Vue is a CJS importable entity, but it's written as ES modules, so how are you transforming your code right now?

Also, what is the original source of breakage for TypeScript users?

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Feb 27, 2017

Hi @DanielRosenwasser . Vue now outputs a es module file for webpack/rollup users. So tree shaking will work. And Vue has supported commonjs output file as well. These two versions are compiled into two files by two different rollup configs.

For node/browserfiy users, they use vue.common.js, specified by main entry in package.json. The export is done via module.exports = Vue and imported by require('vue').

For webpack/rollup users, they use vue.esm.js, specified by module entry in package.json. The export is export default Vue and is imported by import Vue from 'vue'.

Library authors will also want to support both ESM and commonjs. Babel/webpack has compatbile require builtin. If an author writes library in Babel, Babel will take care of how Vue is imported. So library users can freely use ESM/commonjs and the author will need simply write one import Vue from 'vue', regardless of it is executed in node like commonjs environment or webpack like es module environment.

However, TypeScript does not have builtin compatible library. An author must choose one style import Vue from 'vue' or import Vue = require('vue'). TypeScript compiles different code. And different style output is not mutually compatible.

@yyx990803
Copy link
Member Author

yyx990803 commented Feb 27, 2017

After some thoughts, maybe there is something that can be done in webpack 2. Right now the problem is that with the module field present in package.json, webpack will always use it regardless of the import syntax the user is using:

import Vue from 'vue' // <- gets the default export from the ESM build

const Vue = require('vue') // <- returns { __esModule: true, default: Vue }

IMO the module field should only be respected when the user is using ES modules import syntax. If a raw require() is used, it should use the main field instead.

/cc @TheLarkInn

@yyx990803
Copy link
Member Author

And for now, I think the best path forward is recommend Vue + TS users to start using allowSyntheticDefaultImports: true and use ES imports everywhere. This way when this PR lands they don't need to do anything.

@HerringtonDarkholme
Copy link
Member

Let me summarize some findings. Sorted by effort required.

  1. For webpack users: it needs minimal effort, just changing import style is enough.

  2. For library authors: they need to use webpack to compile their libraries for commonjs / esm compatibility. Example.

  3. For plain tsc users/ browserify users: they need use babel for a compatible require.

I'd rather argue the break in case 3 is sooner or later: e.g. by node-esm spec. (My speculation is not many users are using browserify + ts + vue, but if they do use this combo at least babel will come to rescue.)

@yyx990803
Copy link
Member Author

FYI, updated TypeScript usage docs: https://vuejs.org/v2/guide/typescript.html

@DanielRosenwasser
Copy link

DanielRosenwasser commented Feb 28, 2017

And for now, I think the best path forward is recommend Vue + TS users to start using allowSyntheticDefaultImports: true and use ES imports everywhere. This way when this PR lands they don't need to do anything.

The problem is that TypeScript doesn't actually synthesize the default export for a CJS module - this will type-check just fine, but will not work at runtime. allowSyntheticDefaultImports only assumes that you have a loader that will do the right thing (e.g. SystemJS). Does Webpack provide this functionality? Otherwise users will need to run their output through Babel.

@yyx990803
Copy link
Member Author

yyx990803 commented Feb 28, 2017

@DanielRosenwasser for webpack 1 yes the user would have to use babel for interop. Webpack 2 uses the ES module build automatically so the semantics align. Webpack 1 is considered deprecated and upgrading to 2 doesn't require much effort, so we'd prefer sticking to es modules in all cases.

@DanielRosenwasser
Copy link

Sure - it'd be AMD and Webpack 1 that would have problems though. I don't know how many people are using the former, but I'll take your word on the latter. I see you already added allowSyntheticDefaultImports to the documentation, so that should help people there.

@yyx990803
Copy link
Member Author

@DanielRosenwasser while we are on this topic - is it possible to allow packages to configure a different d.ts for different module formats? Similar to how the convention for webpack 2 is using "module" for ES modules build and "main" for CommonJS build, it would be helpful to have something like maybe "module-types" for d.ts with es style exports... or maybe something that allows the user to manually specify the d.ts for a specific module in tsconfig.json, like "resolve.alias" in webpack.

@DanielRosenwasser
Copy link

or maybe something that allows the user to manually specify the d.ts for a specific module in tsconfig.json, like "resolve.alias" in webpack.

This sort of works if you use path-mapping (using the paths option in your tsconfig.json), but it ends up feeling odd. If this ends up being part of the CommonJS interop spec (I'm on a cell phone so this is hard to look up right now) then we will need to do it (cc @mhegazy @vladima).

@yyx990803
Copy link
Member Author

Closing in favor of #5887.

@yyx990803 yyx990803 closed this Jul 1, 2017
@blake-newman blake-newman deleted the typings-refactor branch October 21, 2017 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants