Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Aug 12, 2021

✌️

/cc @zendesk/vegemite

Description

Your code does not properly work as a Node.js module. Now, even though it's not published to NPM, you can at least include the git repository as a node.js dependency and it will work.

Tasks

  • Include comments/inline docs where appropriate
  • Add unit tests
  • Update changelog here

References

DevQA Steps

NOTE: DevQA steps are to be actioned only once code has been reviewed and approved.

Risks

  • [HIGH | medium | low] Does it work across browsers (including IE!)?
  • [HIGH | medium | low] Does it work in the different products (Support, Chat, Connect)?
  • [HIGH | medium | low] Are there any performance implications?
  • [HIGH | medium | low] Any security risks?
  • [HIGH | medium | low] What features does this touch?

Rollback Plan

  1. Quickly roll back to the prior release so that customers can refresh to resolve their issue.
  2. Revert this PR to restore the master branch to a deployable green state.
  3. Notify the author.

"engines": {
"node": ">=12.10.0",
"npm": ">=5.6.0",
"yarn": "Yarn is disabled for this project, use npm instead"
Copy link
Author

@ghost ghost Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't ever do this ever again. It doesn't just prevent local npm install from using yarn, but it prevents all users of your package from using yarn.

"yarn": "Yarn is disabled for this project, use npm instead"
"npm": ">=5.6.0"
},
"babel": {},
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in the version of Babel this project uses, this was interfering with downstream configs; since it doesn't define a config for the package itself, it tries to go up to configs for the parent project, which will fail since you're using an old version of babel.

@@ -1,9 +1,11 @@
/* global URL */
import version from 'version'
import pkgJson from '../package.json'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This builds to the same code with optimisations enabled but doesn't require webpack as an intermediary, meaning that the entire module can be bundled into another webpack bundle with source maps intact.

"name": "zendesk_app_framework_sdk",
"version": "2.0.33",
"main": "lib/zendesk-app-framework-sdk.js",
"main": "lib/index.js",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just wrong before

@kyleyee23
Copy link

+1, short of Zendesk publishing as a Node module, this got my app working

@bkw
Copy link

bkw commented Dec 5, 2024

While #172 may have temporarily enabled importing this as a module, the introduction of synk protect as a prepare script in 5114829 broke this for everybody without a synk subscription, resulting in the error Authentication failed. Please check the API token on https://snyk.io on install:

$ pnpm add https://github.com/zendesk/zendesk_app_framework_sdk --save-dev
../../../Library/pnpm/store/v3/tmp/_tmp_51495_28b55e3e0bf988cb1edb299d770fa32d [[email protected]]: Running npm-install script, failed in 11.2s
...1495_28b55e3e0bf988cb1edb299d770fa32d npm-install$ npm install
│ npm warn deprecated [email protected]: fsevents 1 will break on node v14+ and could be using insecure binaries. Upgrade to fsevents 2.
│ npm warn deprecated [email protected]: Critical bug fixed in v3.0.1, please upgrade to the latest version.
│ npm warn deprecated [email protected]: Please update to ini >=1.3.6 to avoid a prototype pollution issue
│ npm warn deprecated [email protected]: Critical bug fixed in v3.0.1, please upgrade to the latest version.
│ npm warn deprecated [email protected]: Please see https://github.com/lydell/urix#deprecated
│ npm warn deprecated [email protected]: https://github.com/lydell/resolve-url#deprecated
│ npm warn deprecated [email protected]: See https://github.com/lydell/source-map-url#deprecated
│ npm warn deprecated [email protected]: CircularJSON is in maintenance only, flatted is its successor.
│ npm warn deprecated [email protected]: Chokidar 2 does not receive security updates since 2019. Upgrade to chokidar 3 with 15x fewer dependencies
│ npm warn deprecated [email protected]: Debug versions >=3.2.0 <3.2.7 || >=4 <4.3.1 have a low-severity ReDos regression when used in a Node.js environment. It is recommended you upgrade to 3.2.7 or 4.3.1. (https://github.com/visionmedia/debug/issues/797)
│ npm warn deprecated [email protected]: Debug versions >=3.2.0 <3.2.7 || >=4 <4.3.1 have a low-severity ReDos regression when used in a Node.js environment. It is recommended you upgrade to 3.2.7 or 4.3.1. (https://github.com/visionmedia/debug/issues/797)
│ npm warn deprecated [email protected]: Debug versions >=3.2.0 <3.2.7 || >=4 <4.3.1 have a low-severity ReDos regression when used in a Node.js environment. It is recommended you upgrade to 3.2.7 or 4.3.1. (https://github.com/visionmedia/debug/issues/797)
│ npm warn deprecated [email protected]: Chokidar 2 does not receive security updates since 2019. Upgrade to chokidar 3 with 15x fewer dependencies
│ npm warn deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
│ npm warn deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
│ npm warn deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
│ npm warn deprecated [email protected]: See https://github.com/lydell/source-map-resolve#deprecated
│ npm warn deprecated [email protected]: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x. (Note that the API surface has changed to use Promises in 1.x.)
│ npm warn deprecated [email protected]: core-js@<3.4 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js.
│ npm warn deprecated [email protected]: core-js@<3.4 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js.
│ npm warn deprecated [email protected]: core-js@<3.4 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js.
│ > [email protected] prepare
│ > npm run snyk-protect
│ > [email protected] snyk-protect
│ > snyk protect
│ Authentication failed. Please check the API token on https://snyk.io
│ npm error code 2
│ npm error path /Users/demo/Library/pnpm/store/v3/tmp/_tmp_51495_28b55e3e0bf988cb1edb299d770fa32d
│ npm error command failed
│ npm error command sh -c npm run snyk-protect
│ npm error A complete log of this run can be found in: /Users/demo/.npm/_logs/2024-12-05T15_10_06_233Z-debug-0.log
└─ Failed in 11.2s at /Users/demo/Library/pnpm/store/v3/tmp/_tmp_51495_28b55e3e0bf988cb1edb299d770fa32d
 ERR_PNPM_PREPARE_PACKAGE  Failed to prepare git-hosted package fetched from "https://codeload.github.com/zendesk/zendesk_app_framework_sdk/tar.gz/0a86f34fae4e184f62218df70f06a4116bc83ca5": [email protected] npm-install: `npm install`
Exit status 2

CC @etanxing

Even when logged into a snyk account, the build fails. Moreover, I suspect it even wouldn't work for you guys, since:

⚠ WARNING: Snyk protect was removed at 31 March 2022.
Please use '@snyk/protect' package instead: https://updates.snyk.io/snyk-wizard-and-snyk-protect-removal-224137 

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.

2 participants