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

PleaseUpgradeNode issues with NCC/Webpack #1274

Closed
4 of 10 tasks
barlock opened this issue Jan 19, 2022 · 3 comments · Fixed by #2221
Closed
4 of 10 tasks

PleaseUpgradeNode issues with NCC/Webpack #1274

barlock opened this issue Jan 19, 2022 · 3 comments · Fixed by #2221
Assignees
Labels
discussion M-T: An issue where more input is needed to reach a decision enhancement M-T: A feature request for new functionality good first issue Good for newcomers
Milestone

Comments

@barlock
Copy link
Contributor

barlock commented Jan 19, 2022

Description

When building a bolt app with ncc (which uses webpack) pleaseUpgradeNode in the index breaks as it can't find your package.json. I have a monorepo and it picked up my root package.json (which didn't have engines in it).

Error:

/dist/webpack:/node_modules/please-upgrade-node/index.js:5
  var requiredVersion = pkg.engines.node.replace('>=', '')
^
TypeError: Cannot read property 'replace' of undefined
    at pleaseUpgradeNode (/dist/webpack:/node_modules/please-upgrade-node/index.js:5:1)
    at Object.36141 (/dist/webpack:/node_modules/@slack/bolt/dist/index.js:20:1)

Possible solutions:

It strikes me that please-upgrade-node is designed for CLI tools, not libraries. Simply embedding the engine config into the index would work great, It does introduce some duplication though. You distribute your own webpack'd version of the library, maybe you can build it in? Lots of good solutions out there.

After combing through the generated files I found a workaround for me that I just need to add an engines.node block into my own package, but that wasn't the intentions of the bolt authors I assume.

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • example code related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Reproducible in:

NA

Steps to reproduce:

  1. Take an example bolt app,
  2. build it with ncc into dist (something like ncc build src/index.ts -o dist)
  3. node dist/index.js
  4. See error above

Expected result:

The app should run as normal

Actual result:

The error at the top

Attachments:

Logs, screenshots, screencast, sample project, funny gif, etc.

@filmaj filmaj self-assigned this Jan 19, 2022
@filmaj filmaj added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented discussion M-T: An issue where more input is needed to reach a decision question M-T: User needs support to use the project labels Jan 19, 2022
@filmaj
Copy link
Contributor

filmaj commented Jan 19, 2022

Hey @barlock thanks for writing in.

I can't reproduce with the repro steps you provided:

13:56:00 in ~/src/plzupgradenodeslapp is 📦 v1.0.0
➜ cat src/app.js
const { App } = require('@slack/bolt');

// Initializes your app with your bot token and signing secret
const app = new App({
  token: process.env.SLACK_BOT_TOKEN,
  signingSecret: process.env.SLACK_SIGNING_SECRET
});

(async () => {
  // Start your app
  await app.start(process.env.PORT || 3000);

  console.log('⚡️ Bolt app is running!');
})();

13:56:08 in ~/src/plzupgradenodeslapp is 📦 v1.0.0
➜ ncc build src/app.js -o dist
ncc: Version 0.33.1
ncc: Compiling file index.js into CJS
1718kB  dist/index.js
1718kB  [2479ms] - ncc 0.33.1

13:56:15 in ~/src/plzupgradenodeslapp is 📦 v1.0.0 took 2s
➜ node dist/index.js
⚡️ Bolt app is running!

I also tried removing the package.json from the root of my app but the app still works fine.

Any tips on how best I can reproduce the issue?

@filmaj filmaj added needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info and removed bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented discussion M-T: An issue where more input is needed to reach a decision labels Jan 19, 2022
@barlock
Copy link
Contributor Author

barlock commented Jan 19, 2022

I made a repo to reproduce: https://github.com/barlock/bolt-issue-repro

After cloning:

  1. Install deps: yarn install
  2. use ncc to compile: yarn build
  3. start the built app and get error: yarn start

I'm honestly not sure what the difference is between your steps and mine. I thought it might be typescript, but that doesn't appear to be it. I'll be curious if you can reproduce with my repo.

@filmaj
Copy link
Contributor

filmaj commented Jan 19, 2022

Thanks, indeed I can repro now.

Looks like the project's package json gets copied and injected into the bundle generated by ncc.

I'm not sure duplicating the engines content into the index is a nice change 😞 . We don't actually use webpack to build and distribute this library, only tsc, so I don't think building with webpack is a viable approach either. Both feel like workarounds for how bundlers mess with this project's code in non-standard ways.

I'm open to solving this issue but also don't want to bend over backwards just to appease whatever new bundler's non-standard module resolution approach happens to come along. If you have suggestions, we're open to ideas!

@filmaj filmaj added discussion M-T: An issue where more input is needed to reach a decision and removed needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info labels Jan 19, 2022
@seratch seratch added this to the 3.x milestone Jan 25, 2022
@seratch seratch added enhancement M-T: A feature request for new functionality good first issue Good for newcomers and removed question M-T: User needs support to use the project labels Jan 25, 2022
@filmaj filmaj modified the milestones: 3.x, 3.21.2 Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision enhancement M-T: A feature request for new functionality good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants