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

Add errors/warning where we're sure that an error could be #10

Open
timocov opened this issue Jun 29, 2020 · 7 comments
Open

Add errors/warning where we're sure that an error could be #10

timocov opened this issue Jun 29, 2020 · 7 comments
Labels
enhancement New feature or request
Milestone

Comments

@timocov
Copy link
Owner

timocov commented Jun 29, 2020

Related to #8 (comment)

@timocov timocov added the enhancement New feature or request label Jun 29, 2020
@timocov
Copy link
Owner Author

timocov commented Jun 30, 2020

Needs to revert changes for #5 and warn about using in operator for internal types (that's ok for public types).

timocov added a commit to tradingview/lightweight-charts that referenced this issue Jun 30, 2020
…perties in it

We can safely use `in` operator with "public" (or "external") API, but not with internal one.

- See bc0face
- See timocov/ts-transformer-properties-rename#10
@timocov
Copy link
Owner Author

timocov commented Jun 30, 2020

Depends on microsoft/TypeScript#36733

@timocov
Copy link
Owner Author

timocov commented Jul 4, 2020

Quite possible that we need to revert changes from dc11196 and warn about defining properties because at least terser doesn't support properties renaming defined in Object.defineProperty. From the docs https://terser.org/docs/cli-usage#cli-mangling-property-names-mangle-props:

Avoid calling functions like defineProperty or hasOwnProperty, because they refer to object properties using strings and will break your code if you don't know what you are doing.

So I guess we need to check whether a compilation require to use defineProperty to define properties and whether this property is internal and then warn about that.

Terser supports mangling properties defined in defineProperty so it's irrelevant.

/cc @artfiedler

@timocov timocov added this to the 1.0 milestone Jul 4, 2020
@artfiedler
Copy link

artfiedler commented Jul 4, 2020 via email

@timocov
Copy link
Owner Author

timocov commented Jul 4, 2020

I believe those docs are out of date, I had verified terser worked, I had the latest version of terser

Yep, I've tested it after posting my comment and it works fine.

@timocov
Copy link
Owner Author

timocov commented Jul 4, 2020

Related to terser/terser#748

@fabiosantoscode
Copy link

Avoid calling functions like defineProperty or hasOwnProperty, because they refer to object properties using strings and will break your code if you don't know what you are doing.

This is merely to warn people who don't understand the limitations. I got tons of issues which assumed Terser did full flow analysis (which it doesn't).

Ideally I would love property mangle to work more safely, and using typescript typing information to achieve this is a great idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants