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

Layer breaker in common/files #37613

Closed
jrieken opened this issue Nov 3, 2017 · 6 comments
Closed

Layer breaker in common/files #37613

jrieken opened this issue Nov 3, 2017 · 6 comments
Assignees
Labels
debt Code quality issues
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Nov 3, 2017

Try to compile just the monaco-editor-bits with a special tsconfig.json-file, see a compile error in files.ts because it uses process. That's again our layering rules because thing in common must be universal, without specific dependencies.

@jrieken
Copy link
Member Author

jrieken commented Nov 3, 2017

tsconfig.json-content in question

{
	"$schema": "http://json.schemastore.org/tsconfig",
	"compilerOptions": {
		"module": "amd",
		"moduleResolution": "classic",
		"noImplicitAny": false,
		"removeComments": false,
		"preserveConstEnums": true,
		"target": "es5",
		"sourceMap": false,
		"experimentalDecorators": true,
		"declaration": true,
		"noImplicitReturns": true,
		"baseUrl": ".",
		"types": []
	},
	"include": [
		"typings/require.d.ts",
		"typings/thenable.d.ts",
		"typings/es6-promise.d.ts",
		"typings/lib.array-ext.d.ts",
		"typings/lib.ie11_safe_es6.d.ts",
		"vs/css.d.ts",
		"vs/monaco.d.ts",
		"vs/nls.d.ts",
		"vs/editor/*",
		"vs/base/common/*",
		"vs/base/browser/*",
		"vs/base/parts/tree/*",
		"vs/base/parts/quickopen/*",
		"vs/platform/*/common/*",
		"vs/platform/*/browser/*"
	],
	"exclude": [
		"node_modules/*"
	]
}

@jrieken jrieken added the debt Code quality issues label Nov 3, 2017
@jrieken
Copy link
Member Author

jrieken commented Nov 3, 2017

Similar, editor.main.ts depends on exports which is only defined in node.d.ts

@rebornix rebornix added this to the November 2017 milestone Nov 15, 2017
@rebornix
Copy link
Member

Moved the file size check to node folder.

exports object is available in AMD/CJS/UMD so before we need to compile Monaco to ESM, it's good to be there. I'd like to have another issue tracking it as to make the syntax off exports or node stuff, we need to

  • use export instead of exports
  • avoid import = require() as well and replace them with asterisk syntax, by merging cb05178 .

Closing it now as the bad layering is fixed.

@jrieken
Copy link
Member Author

jrieken commented Nov 16, 2017

Sorry, I have cramped to issues into one but this still is a problem: #37613 (comment). You can test/verify with vscode$ tsc -p src/tsconfig.monaco.json --noEmit

@rebornix
Copy link
Member

I see, even though exports is available to standard AMD, but since there is no typings for exports so we get error TS2304: Cannot find name 'exports'. We don't see this kind of error as tsconfig.json as we include node typings there.

To get rid of the error message, adding declare var exports: any; is good enough. @jrieken do you have any particular thing to do with tsconfig.monaco.json? If it's just targeting AMD, then it can be an easy one line fix. If we want to target ESM, then we need more tweaks as ESM only supports named exports.

@jrieken
Copy link
Member Author

jrieken commented Nov 17, 2017

I am good with the declare var... solution. The goal of tsconfig.monaco.json is to just describe the monaco editor and its dependencies so that at one point we can have two compilation units, one that targets browsers (including IE11) and one that targets electron/node with the freedom of using more modern JS

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

No branches or pull requests

3 participants