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

cannot write to private class fields if TypeScript class is preloaded #1138

Closed
tve opened this issue Jun 5, 2023 · 5 comments
Closed

cannot write to private class fields if TypeScript class is preloaded #1138

tve opened this issue Jun 5, 2023 · 5 comments
Labels
fixed - please verify Issue has been fixed. Please verify and close.

Comments

@tve
Copy link
Contributor

tve commented Jun 5, 2023

Build environment: any
Moddable SDK version: 3.9.2
Target device: any

Description
Trying to write to a private field of a preloaded & frozen class fails

Steps to Reproduce

  1. Run the main & manifest provided below using mcconfig -d -m -p lin
  2. Observe run-time error
/home/src/moddable/repro-weakmap/main.ts (2) # Exception: set: WeakMap instance is read-only!
  1. Remove the preloading in manifest.json, re-run, the error is gone
  2. Alternatively, change #foo to foo, re-run, the error is gone too

Other information
I suspect this is just the way it is due to the way private fields are implemented. If so, it would be good to document this limitation in moddable/tree/public/documentation/xs/preload.md

main.js:

class MySensor {
  foo

  constructor() {
    this.foo = 123
  }
}
Object.freeze(MySensor.prototype)

export default function main() {
  const sensor = new MySensor()
  trace("done\n")
}

manifest.json:

{
  "include": [
    "$(MODDABLE)/examples/manifest_base.json",
    "$(MODDABLE)/examples/manifest_typings.json",
    "$(MODDABLE)/modules/io/manifest.json"
  ],
  "modules": {
    "*": ["./*.ts"]
  },
  "preload": ["main"]
}
@phoddie
Copy link
Collaborator

phoddie commented Jun 5, 2023

XS supports private fields in a preloaded class. You can confirm that by changing the externs of your main.ts to ".js" and doing a clean build.

What's going on? The error reported is about an immutable WeakMaps. XS does not implement private fields using WeakMaps. And your code doesn't use a WeakMap directly. But TypeScript emulates private fields using them. Here's the code output by the TypeScript compiler for your project:

var __classPrivateFieldSet = (this && this.__classPrivateFieldSet) || function (receiver, state, value, kind, f) {
    if (kind === "m") throw new TypeError("Private method is not writable");
    if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a setter");
    if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot write private member to an object whose class did not declare it");
    return (kind === "a" ? f.call(receiver, value) : f ? f.value = value : state.set(receiver, value)), value;
};
var _MySensor_foo;
class MySensor {
    constructor() {
        _MySensor_foo.set(this, 123);
        __classPrivateFieldSet(this, _MySensor_foo, 123, "f");
    }
}
_MySensor_foo = new WeakMap();
// Object.freeze(MySensor.prototype)
export default function main() {
    const sensor = new MySensor();
    trace("done\n");
}
//# sourceMappingURL=main.js.map

The solution is to get TypeScript to use native engine support for private fields. I'm sure that's some adjustment to the tsconfig.json. I tried bumping ahead to ES2022, but that didn't help. Maybe you know how?

@phoddie phoddie changed the title cannot write to private class fields if class is preloaded and frozen cannot write to private class fields if TypeScript class is preloaded Jun 5, 2023
@tve
Copy link
Contributor Author

tve commented Jun 5, 2023

Thanks for checking this out!

@phoddie
Copy link
Collaborator

phoddie commented Jun 5, 2023

Good news: it is enough to update to ES2022. I just did it wrong the first time.

Try updating mcmanifest.js:

	generate(tool) {
		let json = {
			compilerOptions: {
				baseUrl: "./",
				forceConsistentCasingInFileNames: true,
				module: "es2022",
				outDir: tool.modulesPath,
				paths: {
				},
				lib: ["es2022"],
				sourceMap: true,
				target: "ES2022",
				...tool.typescript.tsconfig?.compilerOptions
			},

And rebuild tools so that is used. I'm not sure if ES2023 is supported yet by tsc.

@tve
Copy link
Contributor Author

tve commented Jun 5, 2023

yup, thanks! just came to the same conclusion...
thoughts about updating mcmanifest.js in the release?

@phoddie
Copy link
Collaborator

phoddie commented Jun 5, 2023

thoughts about updating mcmanifest.js in the release

Already committed. Will roll out later in the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed - please verify Issue has been fixed. Please verify and close.
Projects
None yet
Development

No branches or pull requests

2 participants