-
-
Notifications
You must be signed in to change notification settings - Fork 540
Re-add "exports" declaration to package.json in backwards-compatible way #1028
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
Conversation
src/index.spec.ts
Outdated
@@ -4,10 +4,12 @@ import { join } from 'path' | |||
import semver = require('semver') | |||
import ts = require('typescript') | |||
import proxyquire = require('proxyquire') | |||
import { register, create, VERSION } from './index' | |||
import * as tsNodeTypes from './index' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type-only import, since we want to require() the locally-installed ts-node later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import type
possibly?
src/index.spec.ts
Outdated
testsDirRequire.resolve('ts-node/register/transpile-only') | ||
testsDirRequire.resolve('ts-node/register/type-check') | ||
testsDirRequire.resolve('ts-node/tsconfig.schema.json') | ||
testsDirRequire.resolve('ts-node/tsconfig.schemastore-schema.json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot load them, because they do things like install require hooks. But resolving should be enough to know that package.json "exports" is still compatible with legacy require() calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove some of these in the next major version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I've already merged other breaking changes to master, so our next release will be major already.
Do you want me to tweak the "exports"
object to only include a few?
Off the top of my head, here's a whitelist of the ones we should keep, what do you think @blakeembrey ?
ts-node
// only reliably way to ask node for the root path of a dependency is Path.resolve(require.resolve('ts-node/package'), '..')
ts-node/package
// All bin entrypoints for people who need to augment our CLI: `node -r otherstuff ./node_modules/ts-node/dist/bin`
ts-node/dist/bin
ts-node/dist/bin-transpile
ts-node/dist/bin-script
// Must be require()able, obviously
ts-node/register
ts-node/register/files
ts-node/register/transpile-only
ts-node/register/type-check
// Maybe this is useful? I dunno; I'd hate to break someone
ts-node/tsconfig.schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. I think we could safely omit tsconfig.schema
but add it back if requested, but happy to keep it there for anyone using it as a manually referenced schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the reason we ship with tsconfig.schema
at all is because of $ref
or $schema
in tsconfig.json
? Maybe we could just omit it and have people reference a static version hosted outside the package going forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's so you can add "$schema": "./node_modules/ts-node/tsconfig.schema.json"
to your tsconfig file and know you are using the correct schema for the version of ts-node you are using. For example, if you're using an old or experimental build that changes which ts-node flags you are allowed to embed in a tsconfig, you'll want to refer to the bundled schema. Even then, you're probably referencing it via filename, not require()
.
For everyone else, their editor is using the schemastore schema by default, which already includes our stuff.
I'll omit it from "exports" and from the tests.
compilerOptions: { | ||
jsx: 'preserve' | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ts-node is now being loaded inside a before()
callback, we must also register ts-node inside a before()
callback.
@@ -637,10 +668,11 @@ describe('ts-node', function () { | |||
}) | |||
|
|||
describe('JSX preserve', () => { | |||
let old = require.extensions['.tsx'] // tslint:disable-line | |||
let old: (m: Module, filename: string) => any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be moved inside a before()
callback.
Codecov Report
@@ Coverage Diff @@
## master #1028 +/- ##
==========================================
+ Coverage 73.68% 74.83% +1.15%
==========================================
Files 6 6
Lines 608 608
Branches 142 142
==========================================
+ Hits 448 455 +7
+ Misses 101 100 -1
+ Partials 59 53 -6
Continue to review full report at Codecov.
|
src/index.spec.ts
Outdated
testsDirRequire.resolve('ts-node/register/transpile-only') | ||
testsDirRequire.resolve('ts-node/register/type-check') | ||
testsDirRequire.resolve('ts-node/tsconfig.schema.json') | ||
testsDirRequire.resolve('ts-node/tsconfig.schemastore-schema.json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove some of these in the next major version?
src/index.spec.ts
Outdated
@@ -4,10 +4,12 @@ import { join } from 'path' | |||
import semver = require('semver') | |||
import ts = require('typescript') | |||
import proxyquire = require('proxyquire') | |||
import { register, create, VERSION } from './index' | |||
import * as tsNodeTypes from './index' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import type
possibly?
"./dist/bin-transpile": "./dist/bin-transpile.js", | ||
"./dist/bin-transpile.js": "./dist/bin-transpile.js", | ||
"./dist/bin-script": "./dist/bin-script.js", | ||
"./dist/bin-script.js": "./dist/bin-script.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide these as exports or only allow node node_modules/ts-node/dist/bin
style? Do we want both .js
and without?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure how people are using ts-node
in the wild, and I couldn't think of any compelling benefits that outweighed the possibility of breaking something. Beyond that I don't have a strong opinion one way or the other.
See #1026