For scope hoisting, Asset IDs cannot contain + or / (base64)

[mischnic]

↪️ Pull Request

Asset IDs are base64 encoded, so + and / are also possible. The IDs are used for hoisting imports and exports, but with an ID kA+M then $kA+M$exports is not a valid JS identifier, causing an error (see issue): Unexpected token, expected ; (2:19)

Collisions might have become more likely now, but shouldn't be that significant?

Fixes #1971

💻 Examples

https://github.com/njscholfield/new-tracks/tree/8eba7b08f286cac510d55e5d85800d64633dd3fe

✔️ PR Todo

• Added/updated unit tests for this change
• Filled out test instructions (In case there aren't any unit tests)
• Included links to related issues/PRs

[DeMoorJasper]

Perhaps it would be a better idea to just not base64 encode it? Base64 has a bunch of invalid variable characters including +, -, =, /

_{Sent with GitHawk}

[DeMoorJasper]

Hex should always be valid variable names if preceded by a letter

_{Sent with GitHawk}

[mischnic]

Perhaps it would be a better idea to just not base64 encode it? Base64 has a bunch of invalid variable characters including +, -, =, /

= are at the end and we only use the first 4 chars of the string. The variant of base64 that Node uses seems to be a-z, A-Z, 0-9, + and /.

What would be an alternative to base64? With base64's many possible chars, collisions are less likely than with hex.

MD5 hashes should always be valid variable names if preceded by a letter

The "only letters at beginning" is not an issue, because scope hoisting always puts a dollar sign in front. And the md5 is encoded as base64:

parcel/packages/core/parcel-bundler/src/utils/md5.js

Lines 4 to 9 in a1567e1

	function md5(string, encoding = 'hex') {
	return crypto
	.createHash('md5')
	.update(string)
	.digest(encoding);
	}

parcel/packages/core/parcel-bundler/src/Asset.js

Line 199 in a1567e1

? md5(this.relativeName, 'base64').slice(0, 4)

[DeMoorJasper]

I know it’s currently being encoded just thought hex would probably be more reliable to prevent special characters.
you’re right about collisions unless you’d use the entire hash hex.
I’d just use the entire hash hex and run a minifier over the final result to create shorter variablenames and some more optimisation but thats a diff discussion.

This PR is probably sufficient for what it’s aiming a, but I don’t think 4 char base64 var names will be sufficient in the long term, seems like collisions and issues are inevitable.

If I remember correctly I even had a PR open to use larger names a lil while back because there were issues with large apps

_{Sent with GitHawk}

[devongovett]

Hmm we also use the toIdentifier function from Babel for all of the scope hoisting generated variable names, which should be removing those bad characters. Perhaps you ran into a case where we forgot to do that?

[mischnic]

Hmm we also use the toIdentifier function from Babel for all of the scope hoisting generated variable names, which should be removing those bad characters. Perhaps you ran into a case where we forgot to do that?

Yes, was missing in VueAsset

[mischnic]

If I remember correctly I even had a PR open to use larger names a lil while back because there were issues with large apps

This one ? #1803 / 804a8a8 + 2bdb00b

[DeMoorJasper]

@mischnic yes that’s the PR

_{Sent with GitHawk}

[n0v1]

I just ran into this error while trying out parcel to bundle a vue app. I did not notice that this fix is just a few days old and not released yet.

In case anyone is interested, here is a small repo to reproduce this error. See the readme for steps to reproduce.

Renaming the component works around this problem but leads to another one (ReferenceError: $rLiX$$interop$default is not defined). This seems to be caused by importing svg inline in combination with --experimental-scope-hoisting.

[mischnic]

Renaming the component works around this problem but leads to another one (ReferenceError: $rLiX$$interop$default is not defined). This seems to be caused by importing svg inline in combination with --experimental-scope-hoisting.

Could you please open an issue for this, if it doesn't already exist?