Conflicting asset ids leads to wrong file being required

[pselden]

🐛 bug report

I ran into an issue only seen when building in production that when lodash's clone method was called it would lead to the following error:

_baseClone.js:142 Uncaught TypeError: e is not a constructor
    at Y (_baseClone.js:142)
    at n (clone.js:33)
    at r.get (memo.js:123)
    at e.get (Transaction.js:53)
    at On (mobx.module.js:3120)
    at e.computeValue (mobx.module.js:1602)
    at e.trackAndCompute (mobx.module.js:1591)
    at e.get (mobx.module.js:1550)
    at e.<anonymous> (mobx.module.js:3479)
    at e.get (mobx.module.js:1055)

I tracked it down to this line here: https://github.com/lodash/lodash/blob/master/.internal/baseClone.js#L200 and added some extra logging around it. What I found was that the "Stack" variable, which should be a reference to an imported lodash function, was actually the string '/authorize_change_trust.5c3cd8f3.png', which is the generated filename for one of the images that import in another file.

I tracked it down further and found that the requires map to the same ID:

"./authorize_change_trust.png":"3+VT"
"./_Stack":"3+VT"

So somehow, only in the production build, these things are being mapped to the same id. Everything is fine in dev/watch mode.

🎛 Configuration (.babelrc, package.json, cli command)

.babelrc

{
  "plugins": [
    [
      "direct-import",
      [
        "@material-ui/core",
        "@material-ui/icons",
        "react-router",
        "react-router-dom"
      ]
    ],
    "transform-decorators-legacy",
    "transform-export-extensions",
    "transform-class-properties",
    "transform-object-rest-spread",
    [
      "babel-plugin-transform-builtin-extend",
      {
        "globals": ["Error"]
      }
    ]
  ],
  "presets": [
    [
      "env",
      {
        "targets": {
          "browsers": ["last 2 versions", "> 1%"]
        },
        "useBuiltIns": "entry"
      }
    ],
    "react"
  ]
}

💻 Code Sample

This is happening here: https://github.com/stellarguard/stellarguard but it's happening in a dependency of this project, not directly in the call site.

I will try to come up with a smaller reproduction step but I'm not sure I'll be able to since the bug probably relies on a lot of little things falling into place to cause a clash.

🌍 Your Environment

Software	Version(s)
Parcel	1.10.3 -- note: this was NOT happening in 1.6.2, which is what I upgraded from, but does happen in 1.9
Node	10.10.0
npm/Yarn	yarn 1.7.0
Operating System	OSX Sierra

[pselden]

So i dug a bit and I guess in production mode it slices off the first 4 characters of an md5 hash of the relative filename (https://github.com/parcel-bundler/parcel/blob/master/packages/core/parcel-bundler/src/Asset.js#L199)... and these two:

../../node_modules/stellar-sdk/node_modules/lodash/_Stack.js'
pages/misc/help/interstellar-exchange/authorize_change_trust.png

Both end up with the same value.

[pselden]

Did some math for fun: in a project that includes 2000 files (around my size) there is a 0.012% chance that this will happen!

[pselden]

I'm not sure if it's worth it to think of a fix since I can just change the filename, but maybe we could warn or throw during build if the asset ids exist aready but the names don't match.

[DeMoorJasper]

This should fix this: #1803
I still need to port this over to the monorepo

[vudzero]

It looks like I do also fall under the 0.012%! :) Renaming my file fix the issue but still not ideal solution.

[lukejagodzinski]

This bug is still present in 1.12.3. For filenames:

• components/rules/EasySelectorField.tsx
• components/actions/ActionForm.tsx

it generates the same ID = ij1t

const crypto = require('crypto');
const fs = require('fs');

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

[
  "components/rules/EasySelectorField.tsx",
  "components/actions/ActionForm.tsx"
].forEach(file => {
  console.log(file, md5(file, "base64").slice(0, 4));
});

It's a little bit frustrating

[lukejagodzinski]

@DeMoorJasper you wrote in the PR:

Although very unlikely it was possible to have duplicate ids.

This PR makes the bundler check for a duplicate and pick a new id if there is a duplicate.

Only necessary on production builds as development uses relative paths and scope hoisting minifies everything anyways

Is this code responsible for generating IDs? https://github.com/parcel-bundler/parcel/blob/master/packages/core/parcel-bundler/src/Asset.js#L204

If so, then I would argue that it's very likely that we can have duplicates of IDs.

    if (!this.id) {
      this.id =
        this.options.production || this.options.scopeHoist
          ? md5(this.relativeName, 'base64').slice(0, 4)
          : this.relativeName;
    }

Just 4 characters long ID/hash is not enough and I don't see any code responsible for checking duplicates

[DeMoorJasper]

@lukejagodzinski yes it's possible, it's just unlikely (on small projects) once you have a large project or are a bit unlucky you can def have duplicates.
Had this on a couple projects already.

Feel free to look at my PR and open up a new PR with the same/similar code

[lukejagodzinski]

@DeMoorJasper it's a huge PR and I just can't find file that makes a change. Could you point me to the commit that checks for duplicates?

[DeMoorJasper]

It's actually pretty small, we just kinda broke Github changed files with a force push to master.

These are the commits that contain all the code changes:
The code: 804a8a8
The test: 2bdb00b

[lukejagodzinski]

@DeMoorJasper Thanks! I will take a look at it and propose my solution. It looks like this change is not even merged. It was probably overridden by some other PR. Someone just didn't take time to review it

[DeMoorJasper]

@lukejagodzinski yes it never got reviewed/merged

[lukejagodzinski]

@DeMoorJasper hmmm for me it looks good. Actually I wouldn't change anything there. Is there anyone that could just merge it and release new version?

[DeMoorJasper]

@lukejagodzinski impossible as master had a force push that messed up the history. Would have to re-create the PR

[lukejagodzinski]

@DeMoorJasper could you recreate this PR?

[DeMoorJasper]

Sure, just opened it as #2999 it was just a copy paste and it worked

[lukejagodzinski]

Hope it will be reviewed and merged soon :)
Thanks you!

[tibdex]

I also experienced this issue. I applied the patch of #2999 and it solved it. Can I do something to help get this merged in the next bug fix release?

Thks!

[lukejagodzinski]

@tibdex as you can see this issue has Parcel 2 label so unfortunately that rather means it's not gonna be released as the 1.x bugfix... However, I'm curious when is 2.0 gonna be released

[devongovett]

Should be fixed in v2