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.

Sign up for GitHub

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

Jump to bottom

Re-add "exports" declaration to package.json in backwards-compatible way #1028

Merged
merged 12 commits into from May 23, 2020
Merged

Re-add "exports" declaration to package.json in backwards-compatible way #1028

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d58b94b
Re-add "exports" declaration to package.json in backwards-compatible way
cspotcode May 4, 2020
af3e0d9
Merge remote-tracking branch 'origin/master' into ab/fix-packagejson-…
cspotcode May 17, 2020
ceb240f
add package.json "exports" tests
cspotcode May 17, 2020
56b8457
more
cspotcode May 17, 2020
3225918
fix
cspotcode May 17, 2020
8057fb8
fix
cspotcode May 17, 2020
4548dcc
Merge remote-tracking branch 'origin/master' into ab/fix-packagejson-…
cspotcode May 17, 2020
daf12da
fix
cspotcode May 17, 2020
c0fc95d
tweak test to check esm loader entrypoint
cspotcode May 20, 2020
311c008
Merge remote-tracking branch 'origin/master' into ab/fix-packagejson-…
cspotcode May 20, 2020
3877ebc
narrow list of exported files
cspotcode May 20, 2020
f88fa6f
Update index.spec.ts
cspotcode May 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -3,6 +3,11 @@
"version": "8.10.1",
"description": "TypeScript execution environment and REPL for node.js, with source map support",
"main": "dist/index.js",
"exports": {
".": "./dist/index.js",
"./": "./",
"./esm": "./esm.mjs"
},
"types": "dist/index.d.ts",
"bin": {
"ts-node": "dist/bin.js",
Expand Down
48 changes: 40 additions & 8 deletions src/index.spec.ts
View file
Open in desktop
Expand Up @@ -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'
Copy link
Collaborator Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

import type possibly?

import { unlinkSync, existsSync, lstatSync } from 'fs'
import * as promisify from 'util.promisify'
import { sync as rimrafSync } from 'rimraf'
import { createRequire, createRequireFromPath } from 'module'
import Module = require('module')

const execP = promisify(exec)

Expand All @@ -18,13 +20,20 @@ const BIN_SCRIPT_PATH = join(TEST_DIR, 'node_modules/.bin/ts-node-script')

const SOURCE_MAP_REGEXP = /\/\/# sourceMappingURL=data:application\/json;charset=utf\-8;base64,[\w\+]+=*$/

// `createRequire` does not exist on older node versions
const testsDirRequire = (createRequire || createRequireFromPath)(join(TEST_DIR, 'index.js')) // tslint:disable-line

// Set after ts-node is installed locally
let { register, create, VERSION }: typeof tsNodeTypes = {} as any

// Pack and install ts-node locally, necessary to test package "exports"
before(async function () {
this.timeout(30000)
rimrafSync(join(TEST_DIR, 'node_modules'))
await execP(`npm install`, { cwd: TEST_DIR })
const packageLockPath = join(TEST_DIR, 'package-lock.json')
existsSync(packageLockPath) && unlinkSync(packageLockPath)
;({ register, create, VERSION } = testsDirRequire('ts-node'))
})

describe('ts-node', function () {
Expand All @@ -35,6 +44,25 @@ describe('ts-node', function () {
it('should export the correct version', function () {
expect(VERSION).to.equal(require('../package.json').version)
})
it('should export all CJS entrypoints', function () {
// Ensure our package.json "exports" declaration allows `require()`ing all our entrypoints
// https://github.com/TypeStrong/ts-node/pull/1026
testsDirRequire.resolve('ts-node')
testsDirRequire.resolve('ts-node/package')
testsDirRequire.resolve('ts-node/dist/index')
testsDirRequire.resolve('ts-node/dist/bin')
testsDirRequire.resolve('ts-node/dist/bin-transpile')
testsDirRequire.resolve('ts-node/dist/bin-script')
testsDirRequire.resolve('ts-node/dist/bin-script-deprecated')
testsDirRequire.resolve('ts-node/dist/esm')
testsDirRequire.resolve('ts-node/register')
testsDirRequire.resolve('ts-node/register/index')
testsDirRequire.resolve('ts-node/register/files')
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')
Copy link
Collaborator Author

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.

Copy link
Member

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?

Copy link
Collaborator Author

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

Copy link
Member

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.

Copy link
Member

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?

Copy link
Collaborator Author

@cspotcode cspotcode May 20, 2020 •
edited

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.

})

describe('cli', function () {
this.slow(1000)
Expand Down Expand Up @@ -523,11 +551,14 @@ describe('ts-node', function () {
})

describe('register', function () {
const registered = register({
project: PROJECT,
compilerOptions: {
jsx: 'preserve'
}
let registered: tsNodeTypes.Register
before(() => {
registered = register({
project: PROJECT,
compilerOptions: {
jsx: 'preserve'
}
})
Copy link
Collaborator Author

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.

})

const moduleTestPath = require.resolve('../tests/module')
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

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.

let compiled: string

before(function () {
old = require.extensions['.tsx']! // tslint:disable-line
require.extensions['.tsx'] = (m: any, fileName) => { // tslint:disable-line
const _compile = m._compile

Expand All @@ -649,7 +681,7 @@ describe('ts-node', function () {
return _compile.call(this, code, fileName)
}

return old!(m, fileName)
return old(m, fileName)
}
})

Expand Down