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

Update to njs-0.8.0 and use js_shared_dict_zone to minimize file read #21

Merged
merged 6 commits into from
Jul 17, 2023
Merged

Update to njs-0.8.0 and use js_shared_dict_zone to minimize file read #21

merged 6 commits into from
Jul 17, 2023

Conversation

ivanitskiy
Copy link
Contributor

@ivanitskiy ivanitskiy commented Jul 13, 2023

Proposed changes

This PR introduces NJS v0.8.0 and use js_shared_dict_zone to minimize file read. In order to update to NJS we would follow some deprecations and remove workarounds prior NJS v0.8. we have fixed some pain points we had.

Run prettier against more files and ensure npm run lint lints files.

A few notable changes from NJS v0.8.0:

  • introduced js_shared_dict_zone directive
  • non-compliant deprecated String methods were removed
  • The following properties for CryptoKey were added:
    algorithm, extractable, type, usages
  • added Array.from(), Array.prototype.toSorted(),
    Array.prototype.toSpliced(), Array.prototype.toReversed().

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • I have updated any relevant documentation (README.md and CHANGELOG.md)

@ivanitskiy
Copy link
Contributor Author

Copy link
Collaborator

@ryepup ryepup left a comment

Choose a reason for hiding this comment

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

some questions and nits, looks like a really nice improvement.

@@ -25,8 +25,7 @@
"build": "rollup -c --environment NODE_ENV:dev",
"clean": "rimraf dist/* lib/* node_modules/.cache/*",
"lint": "run-p lint:*",
"lint:eslint": "eslint --cache --cache-location node_modules/.cache/eslint --ext .ts,.js .",
"lint:types": "tsc -b",
"lint:eslint": "npx eslint .",
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need the npx? eslint is in our devDependencies so I think it should be accessible as just

Suggested change
"lint:eslint": "npx eslint .",
"lint:eslint": "eslint .",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just following https://typescript-eslint.io/getting-started (as eslint directly did not raise a ton of issues originally, but npx eslint . did.

Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting; I wonder if that's pulling in a newer version than what's in devDependencies. I don't think it's a problem, but might become one later if different tools are using different versions of eslint. JS dev tool setup is kinda an infinite source of work, so I vote we move forward with whatever works and we can iterate in the future as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here how I tested it, by having those

    "lint": "run-p lint:*",
    "lint:eslint": "npx eslint .",
    "lint:eslint2": "eslint .",

then ran it (as you can see lint:eslint2 did not raise issues at all!! ) :

$ npm run lint

> njs-acme@1.0.0 lint
> run-p lint:*


> njs-acme@1.0.0 lint:eslint2
> eslint .


> njs-acme@1.0.0 lint:eslint
> npx eslint .


/Users/ivanitskiy/go/src/github.com/nginxinc/njs-acme-experemental/integration-tests/hooks.ts
  27:26  error  Replace `"22"` with `'22'`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

ERROR: "lint:eslint2" exited with 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally have no idea about npx vs npm. lol

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated
Comment on lines 289 to 277
let cache = false
let path = ''
const prefix = acmeDir(r)
const serverNames = acmeServerNames(r)
const commonName = serverNames[0].toLowerCase()
const zone = acmeZoneName(r)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we have enough to make the decision to cache / use zones right here, and can reduce the mutability.

Suggested change
let cache = false
let path = ''
const prefix = acmeDir(r)
const serverNames = acmeServerNames(r)
const commonName = serverNames[0].toLowerCase()
const zone = acmeZoneName(r)
let path = ''
const prefix = acmeDir(r)
const serverNames = acmeServerNames(r)
const commonName = serverNames[0].toLowerCase()
const zone = acmeZoneName(r)
const cache = zone && ngx.shared && ngx.shared[zone]

The cache will be falsy or the zone, so I think later down we can do things like:

if (cache) {
  data = (cache.get(key) as string) || ''
  if (data) {
    return data
  }
}
// ... code omitted ...
if (cache && data) {
  cache.set(key, data)
}

examples/nginx.conf Show resolved Hide resolved
@@ -40,11 +44,17 @@ http {
# js_var $njs_acme_directory_uri https://pebble/dir;
# js_var $njs_acme_verify_provider_https false;

## advanced use-case
# if whant to use `js_shared_dict_zone` or have already one defined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# if whant to use `js_shared_dict_zone` or have already one defined
# use a `js_shared_dict_zone` to store certs/keys in memory

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ivanitskiy ivanitskiy requested a review from ryepup July 15, 2023 00:11
@ivanitskiy ivanitskiy merged commit 61d1147 into nginx:main Jul 17, 2023
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