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

Unbreak production builds in parcel #493

Closed
wants to merge 1 commit into from
Closed

Conversation

jlfwong
Copy link
Owner

@jlfwong jlfwong commented Jan 14, 2025

Blocks #491

@coveralls
Copy link

Coverage Status

coverage: 43.708%. remained the same
when pulling 1dee0ee on jlfwong/upgrade-parcel-p2
into c7be11d on main.

@jlfwong
Copy link
Owner Author

jlfwong commented Jan 14, 2025

@cerisier Unfortunately the parcel upgrade, as it always does, introduces some changes that create a bunch of pain.

I can't land this yet because scripts/build-release.sh currently fails like so:

+ node_modules/.bin/parcel build assets/index.html --no-cache --dist-dir /Users/jlfwong/code/speedscope/dist/release --public-url ./ --detailed-report
🚨 Build failed.

@parcel/core: Failed to resolve 'preact' from './src/speedscope.tsx'

  /Users/jlfwong/code/speedscope/src/speedscope.tsx:1:25
  > 1 | import {h, render} from 'preact'
  >   |                         ^^^^^^^^
    2 | import {ApplicationContainer} from './views/application-container'
    3 | import {ThemeProvider} from './views/themes/theme'

@parcel/resolver-default: External dependency "preact" is not declared in package.json.

  /Users/jlfwong/code/speedscope/package.json:79:3
    78 |   },
  > 79 |   "dependencies": {
  >    |   ^^^^^^^^^^^^^^
    80 |     "open": "7.2.0"
    81 |   }

Accepting the suggestion would probably fix this bug, but it introduces an IMO unacceptable tradeoff -- it makes the speedscope npm package take on dependencies that are totally unneeded because it doesn't import them at all. This bloats what happens when people run npm install speedscope. The dependencies as listed are correct.

One solution for this is create a totally separate package.json for the CLI published through npm than I use for defining main project dependencies.

An alternative solution would be to complete the work I started in #295 and drop this annoying dependency entirely. I'm going to sleep on it and decide what to do later.

@cerisier
Copy link
Contributor

That's very unfortunate :( Especially since parcel is a bundler...
I wonder if bundledDependencies isn't made for this usecase, since you're bundling preact.
Maybe worth opening an issue on parcel.

@jlfwong
Copy link
Owner Author

jlfwong commented Jan 15, 2025

Abandoning in favor of #432

@jlfwong jlfwong closed this Jan 15, 2025
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.

3 participants