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

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

Merged
merged 3 commits into from Feb 25, 2019

Conversation

mischnic
Copy link
Member

↪️ 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
Copy link
Member

DeMoorJasper commented Feb 25, 2019

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
Copy link
Member

DeMoorJasper commented Feb 25, 2019

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

Sent with GitHawk

@mischnic
Copy link
Member Author

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:

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

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

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Feb 25, 2019

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

@mischnic yes that’s the PR

Sent with GitHawk

@devongovett devongovett merged commit a69de78 into master Feb 25, 2019
@devongovett devongovett deleted the valid-asset-id branch February 25, 2019 18:38
@n0v1
Copy link

n0v1 commented Mar 1, 2019

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
Copy link
Member Author

mischnic commented Mar 1, 2019

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?

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.

None yet

4 participants