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

Separate build/ folder and include both ts source files and compiled js files in the release. #1955

Closed
2 tasks
kanitw opened this issue Feb 16, 2017 · 8 comments

Comments

@kanitw
Copy link
Member

kanitw commented Feb 16, 2017

Currently, we only include js files (and exclude ts files) in the release due to changes in #1649.

However, I think we should instead include both to provide flexibility for both downstream ts and js projects.

  • Move all build files (including vega-lite.js|min.js and all compiled *.js and *.d.ts) to build/
  • Keep the ts src files in the release.
@kanitw
Copy link
Member Author

kanitw commented Feb 16, 2017

microsoft/TypeScript#12358 is relevant -- it's pretty confusing though. I still don't know what's the right solution.

@kanitw
Copy link
Member Author

kanitw commented Feb 16, 2017

Additional research: TypeStrong/ts-loader#278 (comment), maybe we shouldn't do this..

@kanitw
Copy link
Member Author

kanitw commented Feb 16, 2017

After research and some experiment, I think this is the way to go.
For reference, here is what I do in CompassQL to make it work.
https://github.com/vega/compassql/compare/eedcdfaf440fc910fff727c043237f2cbba601c6...0cbbb186d37a58e35a69f7c7f6e2817b2fd4bfcd?expand=1

@kanitw
Copy link
Member Author

kanitw commented Feb 17, 2017

I explained the benefits of this approach from my experiment with CompassQL in here:

https://github.com/vega/compassql/releases/tag/v0.9.2

cc: @weswigham - I wonder if you have any objection against this since this will override some of your previous changes.

@weswigham
Copy link
Contributor

weswigham commented Feb 17, 2017

Keeping the .ts build files in the project is a bad idea, since then any errors in vega-lite are likely to be inherited by project consumers which use the ts, and they also suffer from configuration mismatch issues where, if, for example, they have strictNullChecks on and vega-lite does not, they still receive piles of errors within vega-lite's source. Just because you can load the ts from the node package doesn't make it a good idea. It's actually an awful one. The generated .d.ts files contain any and all of the type information a ts consumer should care about anyway. (And if they don't, then something is wrong with how your types are being exposed)

Just don't include the .ts files in a published package. The ability to even include .ts files from your node_modules folder was due to this PR - the only reason it is done is to support monorepo-like projects (aka 100% consistent settings and only one final intended top-level build target) which shard themselves into node_modules and are entirely written in TS - and that scenario is close to the only valid one for keeping the .ts files in your packages - and even that TS doesn't support terribly well anymore unless you explicitly add the nested module's .ts files to your build, thanks to the outcome of this issue.

In short, it normally causes more problems for library consumers than the library developer ones it solves.

@kanitw
Copy link
Member Author

kanitw commented Feb 17, 2017

I don't think this proposal disagrees with the fact that users should import the js files with .d.ts

As mentioned in https://github.com/vega/compassql/releases/tag/v0.9.2, we will say that users should do targeted import from the output JS in build/ folder.

The original approach you did in an earlier PR (just leave js files in src/) was also problematic with npm link (as dependent TS project will still import linked TS files and have the problems you mentioned).

I think we should still include TS files so users can debug VL related stuff with proper source map linked to the original TS code in dependent projects. Source map is becoming more and more important as TS include more complex syntax like the spread operator and object decomposition-- the compiled JS code now can diverge from the original TS files a lot.

@weswigham
Copy link
Contributor

IMO, there is no really good fix for the npm link scenario unless TS starts recognizing "sub compilations" and their associated settings.

So long as the "types" and "main" entry points in the package.json still point at the built .d.ts and .js files, I don't care too much about what gets done with the .ts - leaving it around is just a likely footgun for someone who doesn't know any better. Make the .ts npm link consumer do the scoped import (and then deal with settings issues) - not everybody else.

@kanitw
Copy link
Member Author

kanitw commented Feb 17, 2017

@weswigham Thanks so much for your input.

I think what I did in CompassQL doesn't violate any of the major concerns you have and works fine when I npm link compassql in a dependent project, so I think I'll apply the same approach here then. :)

@kanitw kanitw closed this as completed Mar 3, 2017
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

No branches or pull requests

2 participants