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

fix(*): use the full hash to avoid collisions #4800

Closed
wants to merge 6 commits into from
Closed

fix(*): use the full hash to avoid collisions #4800

wants to merge 6 commits into from

Conversation

ArsenyYankovsky
Copy link

↪️ Pull Request

Fixes 3823 and 3747. Switching to Parcel 2 is not an option for us since it's in the Beta and doesn't support multi-target builds. I think it's a pretty critical bug fix and should be fixed in the Parcel 1.

💻 Examples

One example of relative paths that will generate same id is:
../../node_modules/lodash-es/clone.js
../../node_modules/core-js/library/modules/_an-object.js

✔️ 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

@height
Copy link

height bot commented Jun 25, 2020

This pull request has been linked to 28 tasks:

💡Tip: Add "Close T-190" to the pull request title or description, to a commit message, or in a comment to mark this task as "Done" when the pull request is merged.

@DeMoorJasper
Copy link
Member

We don't really update parcel 1 anymore, is there any way we can help you transition to Parcel 2 instead?

@ArsenyYankovsky
Copy link
Author

I tried switching to Parcel 2 and almost immediately I found a blocking issue for us which is the absence of multi-target builds. I see that it's in the todo list. I don't think it's good to drop support in the form of bug fixes for Parcel 1 until Parcel 2 is released and considered production-ready.

Thanks for a fast response!

@mischnic
Copy link
Member

What exactly do you mean by "multi-target builds"? Parcel 1 didn't support this either?
Builds with multiple entry points and/or multiple targets do work. It's just that assigning specific entry points to specific targets isn't possible

@ArsenyYankovsky
Copy link
Author

ArsenyYankovsky commented Jun 25, 2020

We are running the following command to build our apps: parcel build ./react/apps/*.tsx --no-content-hash which builds all the apps in that directory.

Using parcel@nightly currently with the same command gives me a following error:

Error: Bundles must have unique filePaths

If I just build one of them it gives me files with names like gulpfile.js and gulpfile.css. I guess I need to configure my entries using the programmatic API?

@mischnic
Copy link
Member

mischnic commented Jun 25, 2020

Error: Bundles must have unique filePaths

This usually happens because you have somehow specified an output path for the output bundle but are using multiple entries (which obviously cannot all have the same output filename).

I guess I need to configure my entries using the programmatic API?

No, does your package.json contain have a main/module/browser field? (Not sure where that gulpfile prefix comes from)

@ArsenyYankovsky
Copy link
Author

Thank you very much, I had main entry in my package.json which I removed and now can successfully build.

However, we need to support IE11 and that means using es5 as target. My output files contain arrow functions and I'm not sure why.

My .babelrc:

{
  "presets": [
    [
      "@parcel/babel-preset-env",
      {
        "forceAllTransforms": true
      }
    ],
    "@babel/preset-typescript"
  ],
  "plugins": [
    [
      "@babel/plugin-proposal-decorators",
      {
        "legacy": true
      }
    ],
    [
      "@babel/plugin-proposal-class-properties",
      {
        "loose": true
      }
    ],
    [
      "@babel/plugin-transform-react-jsx"
    ]
  ]
}

My tsconfig.json:

{
  "compilerOptions": {
    "module": "commonjs",
    "target": "es5",
    "noImplicitAny": false,
    "outDir": "./dist",
    "lib": [
      "es5",
      "DOM"
    ],
    "sourceMap": true,
    "listEmittedFiles": true,
    "allowSyntheticDefaultImports": true,
    "esModuleInterop": true,
    "jsx": "react"
  },
  "esModuleInterop": true,
  "include": [
    "react/**/*"
  ]
}

and my package.json has following section:

  "browserslist": [
    "last 1 version",
    "> 1%",
    "maintained node versions",
    "not dead",
    "IE 10"
  ]

I might've missed something but I can't honestly see why. Thank you again.

@mischnic
Copy link
Member

  • tsconfig is ignored by @babel/preset-typescript.
  • maybe these arrow functions come from node_modules? Your project's babelrc isn't used for npm dependencies.

@ArsenyYankovsky
Copy link
Author

Yes, they actually do come from the node_modules. All the arrow functions from my code are transformed correctly. Should I use overrides section in my .babelrc to process my dependencies or is there a nicer way? Thank you again.

@ArsenyYankovsky
Copy link
Author

I tried using babel.config.js and babel.config.json instead of my .babelrc to make it a project-wide configuration but that didn't seem to change anything.

@mischnic
Copy link
Member

babel.config.* is monorepo-wide, but still not for node_modules.

Ideally, the "includes" field in babelrc would be respected (#1655).

@DeMoorJasper
Copy link
Member

I don't actually think node_modules are being transpiled with babel in Parcel 1 either as far as I remember so it's not really different, but we definitely need to support babel's includes at some point

@ArsenyYankovsky
Copy link
Author

I think parcel 1 uses babel 6 which transpiles node_modules by default. I tried different things but what seems to have worked is using @babel/register. I will try using it to require all my modules tomorrow and will let you know. Thanks again for great support. You guys are awesome.

@ArsenyYankovsky
Copy link
Author

I tried using @babel/register to process my node_modules and it initially complained about reprocessing itself so I ended up ignoring some of the babel and parcel related modules like that:

require('@babel/register')({
  ignore:
    [
      './node_modules/@babel/*',
      './node_modules/@parcel/babel-preset-env/*',
      './node_modules/@parcel/transformer-json/*',
      './node_modules/@parcel/transformer-postcss/*',
      './node_modules/@parcel/transformer-css/*',
      './node_modules/@parcel/packager-js/*',
      './node_modules/@parcel/packager-css/*',
      './node_modules/@parcel/optimizer-cssnano/*',
      './node_modules/@parcel/optimizer-terser/*',
    ],
})

Unfortunately, there are still arrow functions in the resulting file. I'm running build command with the --no-cache flag so there's nothing cached.

@ArsenyYankovsky
Copy link
Author

ArsenyYankovsky commented Jul 10, 2020

I've worked around by removing a dependency that had arrow functions. I had other issues which I applied various workaround to fix. The last thing I encountered is the optimization being so slow it times out on CI (take more than 10 minutes to optimize css). It's been a hassle to make it work and it still doesn't. I still don't understand why bugfixes for parcel 1 can't be merged, especially since parcel 2 is in this state. Guess I'll switch to webpack.

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Jul 11, 2020

@ArsenyYankovsky Parcel 2 is in a state that is almost as stable as Parcel 1, just lacks a bit of docs and plugin api stability. There's also some issues with cache stability but Parcel 1 also had those...

Not sure how optimizations time out CI especially css, must be quite some codebase if that happens and would probably also happen with webpack...

I also totally agree it's pretty ridiculous that this never has been fixed so far, the original fix for this bug had been created 2 years ago when I encountered it and created a PR. This should've been fixed a long time ago... Back then we also still released Parcel 1 updates...

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.

Generated asset id is prone to collisions -- breaking imports
3 participants