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

Dependency Updates #246

Merged
merged 15 commits into from
Jun 7, 2024
Merged

Dependency Updates #246

merged 15 commits into from
Jun 7, 2024

Conversation

elliot-huffman
Copy link
Collaborator

@elliot-huffman elliot-huffman commented Jun 5, 2024

Changelog

  • Match the earliest currently supported LTS version of Node.JS | Node 18 LTS
  • Migrate ESLint/Typescript-ESlint to the new flat config standard in prep for the ESLint 9 migration (after typesccript-eslint v8 is released)
  • Update TypeScript config to the latest version
  • Update Typescript output to match the latest version of ECMAScript supported by Node 18 LTS. (ES2022)
    • Match language feature configuration to the new language target
  • Disable declaration source generation since that goes un-used in dev.
  • Remove comments from the transpiled source output since that is not needed in production and the source maps point back to the typescript files.
  • Update all dependencies to the latest versions available
  • Add compile script for production builds that is production optimized
    • Removes the source maps since the source is not included in the published output
  • Remove Prettier JS and JSON checks since those files don't exists or don't need to be checked.

BREAKING CHANGES

  • New minimum Node.JS version required is 18 LTS!
  • Declaration map files (d.ts.map) are no longer generated (the d.ts files still are though)

Migrate off of the old rc/json config standard to the new flat file config format.
All settings have been retained from before.
The node-modules directory doesn't have to be explicitly called out since it is ignored by ESLint by default.
Update the ignore file to the latest spec provided by GitHub:
https://github.com/github/gitignore/blob/main/Node.gitignore
Re initialize the project so that all of the new configurations are present.
All of the same settings are present with the addition of the new defaults in the latest TS configs.
Since Node 18 LTS is the earliest supported version of Node.JS, match the TS output with the maximum version of ECMAscript that is supported by Node.JS 18 LTS.
Disable declaration source maps since the source maps are already included with the project.
Disable downlevel iteration since ES2022 is the target and it supports modern iteration. This will improve performance.
Remove comments from the transpiled output since they aren't necessary in production, this will reduce bundle size of the final project.
Since the target is now ES2022, the lib config is not needed since the functionality is now included in the language output.
@elliot-huffman elliot-huffman added enhancement New feature or request dependencies Pull requests that update a dependency file labels Jun 5, 2024
@elliot-huffman elliot-huffman self-assigned this Jun 5, 2024
Remove the cross-env command since it was un-used.
The cross-env package is also depricated.
The Node_environment should be set externally, not inline with the test script.
Additionally, it wasn't even used in this project...
Since no JS exists any more, this is failing test runs.
Since the JSON files don't get included in the compiled output.
Additionally, the TS Config file is supposed to follow the official TS specification, not the custom standard we are currently using. The one we use reduces readability compared to the default config.
@elliot-huffman
Copy link
Collaborator Author

@rhys-vdw, I honestly don't know how to fix the latest failing test. Everything except that one test passes, and I can't seem to track down where the generated file shows up and what is expected of it to verify the content is expected. Would you be able to assist me with this?

Update the NPM ignore to support the new flat file ES Lint config.
Replace the prepare script with a new production build optimized build command that doesn't emit sourceMap files. This reduces bundle size at publish time.
Reduce command chaining by splitting the respective pre-execution commands into a "pre" script in the script lifecycle manager.
https://docs.npmjs.com/cli/v10/using-npm/scripts#pre--post-scripts
@rhys-vdw
Copy link
Owner

rhys-vdw commented Jun 5, 2024

Ah yeah, that test is hardcoded to a specific output from UglifyJS, which you probably updated. The purpose of the test is to ensure that the shortcircuit function will result on code stripping (as per thereadme).

Really the check is to make sure the body of the function was stripped due to a constant expression being evaluated as false. Maybe we could change it to check for the lack of presence of a variable name in the output so it's less brittle.

the --ext param is no longer available. The target files are not configured in the linter config itself.
Updated the expected output for the new ES2022 output compatibility that Node 18 brings.
The updated ES2022 logic is related to the module system.
@elliot-huffman
Copy link
Collaborator Author

@rhys-vdw

Got the test fixed. Turns out the new ES2022 target was changing the import/export logic slightly, I updated the hard coded test to match the new output.
Plz take a peek at the total updates and merge if you are happy!

Don't forget to enable socket to do dependency scanning.

@elliot-huffman elliot-huffman marked this pull request as ready for review June 7, 2024 04:53
@rhys-vdw
Copy link
Owner

rhys-vdw commented Jun 7, 2024

Nice one. In a recent project I discovered that you can greatly simplify tsconfig by using the extends command these days. The one in the project is an absolute mess hahaha. Might be worth doing if you're in the mood for further cleaning.

Declaration map files (d.ts.map) are no longer generated (the d.ts files still are though)

Do these have any value? I don't recall deliberately enabling their generation.

Don't forget to enable socket to do dependency scanning.

I have enabled it for this repo now, but only after the tests ran. I'm not sure I can trivially re-run it for this PR.

Otherwise looks great, thanks for doing this drudgework of bringing the project up to speed. Feel free to make other improvements as required, but for anything major I'd prefer an issue were opened for discussion first.

I'm happy to merge this one but I'll let you do the honours. I'll get a notification and can run the release script when it's done.

@elliot-huffman
Copy link
Collaborator Author

elliot-huffman commented Jun 7, 2024

Nice one. In a recent project I discovered that you can greatly simplify tsconfig by using the extends command these days. The one in the project is an absolute mess hahaha. Might be worth doing if you're in the mood for further cleaning.

I am interested in doing further cleaning, I was thinking of migrating the test system to something more mainstream, like Mocha, it will improve the readability of the tests a ton.

I was also thinking about moving everything out of just a single file and into a file/folder structure.

Declaration map files (d.ts.map) are no longer generated (the d.ts files still are though)

Do these have any value? I don't recall deliberately enabling their generation.

Only for a certain type of debugging that I don't expect us to do on our side. I am prepping a new PR for deep VS code integration which actually uses the map files but only the .ts maps, not the .d.ts maps.

I am not aware of anything that you are currently doing that would require them, they 100% aren't useful for end users since you don't ship the *.ts files source code with your NPM packages.

Don't forget to enable socket to do dependency scanning.

I have enabled it for this repo now, but only after the tests ran. I'm not sure I can trivially re-run it for this PR.

Awesome! No worries on not having it here. It will just add security risk context to the PR if it finds anything. I have manually ran all of the deps through socket by hand while I was waiting.

Otherwise looks great, thanks for doing this drudgework of bringing the project up to speed. Feel free to make other improvements as required, but for anything major I'd prefer an issue were opened for discussion first.

No worries, can do :)

I'm happy to merge this one but I'll let you do the honours. I'll get a notification and can run the release script when it's done.

I prefer you to do the merges as I don't want to overstep my permissions that you have granted me.

Technically I can bypass merge reviews since you made me a contributor, if this was not intentional, I can show you have to enforce branch protection policies on the project to prevent people like me from taking over the project :-P

@rhys-vdw
Copy link
Owner

rhys-vdw commented Jun 7, 2024

Only for a certain type of debugging that I don't expect us to do on our side. I am prepping a new PR for deep VS code integration which actually uses the map files but only the .ts maps, not the .d.ts maps.

Oh sorry, I understood this to mean that ts-auto-guard was outputting them. Yeah, they have no use in the build process.

Technically I can bypass merge reviews since you made me a contributor, if this was not intentional, I can show you have to enforce branch protection policies on the project to prevent people like me from taking over the project :-P

Ah that's fine, you don't have the ability to do a release which is the only real security concern IMO.

@rhys-vdw rhys-vdw merged commit b3ae411 into rhys-vdw:master Jun 7, 2024
3 checks passed
@elliot-huffman elliot-huffman deleted the Deps-Update branch June 7, 2024 05:28
@elliot-huffman
Copy link
Collaborator Author

Only for a certain type of debugging that I don't expect us to do on our side. I am prepping a new PR for deep VS code integration which actually uses the map files but only the .ts maps, not the .d.ts maps.

Oh sorry, I understood this to mean that ts-auto-guard was outputting them. Yeah, they have no use in the build process.

Technically I can bypass merge reviews since you made me a contributor, if this was not intentional, I can show you have to enforce branch protection policies on the project to prevent people like me from taking over the project :-P

Ah that's fine, you don't have the ability to do a release which is the only real security concern IMO.

Check your email on that last one. Sent you a security risk notice.

@rhys-vdw
Copy link
Owner

Hey @elliot-huffman, just running the release now. https://github.com/rhys-vdw/ts-auto-guard/actions/runs/9459986476

Will reply to your email in time, bit busy atm. :-)

@rhys-vdw
Copy link
Owner

Ah looks like I need to make some token updates. Will handle in time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request Security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants