Skip to content

Fix large variable count numbering - #2244

Merged
lukastaegert merged 1 commit into
masterfrom
large-var-count
Jun 7, 2018
Merged

Fix large variable count numbering#2244
lukastaegert merged 1 commit into
masterfrom
large-var-count

Conversation

@guybedford

@guybedford guybedford commented Jun 6, 2018

Copy link
Copy Markdown
Contributor

Because of the special treatment of $$ in variable deshadowing, we need to avoid outputting this as a trailer in safe import binding deshadowing. This limits the base output to 63 to avoid the use of $ possibly creating a $$ output.

Fixes #2241.

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the quick fix + test! Just a minor suggestion.

Comment thread src/utils/base64.ts
const chars = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_$';

export function toBase64(num: number) {
export function toBase64(num: number, base63 = false) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any reason why we should not always use base63? As far as I can see, this is only used for deshadowing anyway so there should not be any issues. But using a function toBase64 to convert to base 63 but only in some situations seems a little weird.

@guybedford

Copy link
Copy Markdown
Contributor Author

Variable deshadowing in combined scopes will still use base64 for what it is worth. We could certainly make both base63, just that 64 is a nice number really to keep.

@lukastaegert

Copy link
Copy Markdown
Member

just that 64 is a nice number really to keep

I see. But I checked and we also use it for export minification where the algorithm relies on it being base 64 so ok. Maybe we should separate these concerns at some point.

@lukastaegert lukastaegert added this to the 0.60.1 milestone Jun 7, 2018
@lukastaegert
lukastaegert merged commit 662ee81 into master Jun 7, 2018
@lukastaegert
lukastaegert deleted the large-var-count branch June 7, 2018 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants