Package Refactor (Tests, CI) #46

harshzalavadiya · 2021-01-02T13:04:13Z

@Gozala I saw that this package can have many improvements that will make package maintainance easy for maintainers

Changelog

Tests

Moved from tape to jest since they were failing and test packages are not maintained from a while
Seprated from single test file to multiple test suites so it will be easy to read

Documentation

added JSDoc comments on function for imroved IDE autocomplete

CI

Moved to GitHub Actions since travis-ci.org is closing and ci.testling.com doesn't work anymore
for automated publish via github tagged release please set your NPM_AUTH_TOKEN to Your Repo > Settings > secrets > new repository secret more info here
Publish will be only triggured on tagged release otherwise for other PR/branch it will only run build + tests see https://github.com/harshzalavadiya/querystring/actions

File movements

renamed license.md to LICENSE since this it directly recognized by IDE like vscode as license file
renamed History.md to CHANGELOG.md with changelog guidelines
added .prettierrc.json to keep format consistant across any IDE
added .npmignore Only publish required files #45

I'll be happy to help if you need more explaination or help on this PR

medikoo

@harshzalavadiya great thanks for this! Still please see my comments

Ideally if such improvements are proposed with multiple PR's, each covering different matter (e.g. CI, prettification, meta updates are separate matters that can be handled exclusively in its own PR's)

Such approach will also allow us the take in obvious changes, while elaborate on once which require further clarification.

Anyway we can continue that way, see my comments.

medikoo · 2021-01-04T09:56:05Z

src/decode.js

@@ -21,14 +21,31 @@



Let's not nest files in src folder (there's no transpilation step involved, so there's no point to maintain src/dist division)

medikoo · 2021-01-04T09:56:45Z

package.json

+    "commonjs",
+    "query",


I think those keywords are not relevant to this package

medikoo · 2021-01-04T10:00:04Z

.prettierrc.json

+  "semi": true,
+  "singleQuote": true,
+  "trailingComma": "none"
+}


Let's stick to Prettier defaults (eventually we may put printWidth to 100).

Also let's put this config into package.json

Done + also added preetier command to package.json

medikoo · 2021-01-04T10:02:46Z

CHANGELOG.md

+
+All notable changes to this project will be documented in this file.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),


Can CHANGELOG generation be automated in anyway? Or is it just simply about agreeing on rules and otherwise updating changelog manually?

I think as of now it's manual since they can be different from commit messages (like something deprecated/upgrade instructions etc.)

medikoo · 2021-01-04T10:03:03Z

package.json

@@ -1,62 +1,24 @@
 {
  "name": "querystring",
  "id": "querystring",
-  "version": "0.2.0",
+  "version": "0.2.1",


Let's not bump version in this PR

I have changed it back to v0.2.0 but current version of package currently ships some wierd binary files, so giving minor bump might be a good thing.

I know this will be tree shaken most off time but this will save 15-20 kb on package download

I agree.

Whole point is to handle version release outside of this PR (I believe versioning should not be mixed in features/fixes PR's)

medikoo · 2021-01-04T10:04:01Z

package.json

-    "test-browser": "node ./node_modules/phantomify/bin/cmd.js ./test/common-index.js",
-    "test-node": "node ./test/common-index.js",
-    "test-tap": "node ./test/tap-index.js"
+    "test": "jest"


This package was tested in browser engines, while if I see correctly it's no longer the case after this change

true, like earlier browser testing isn't done since jest uses jsdom by default, if not jest then also any suggestion is welcome :)

Was testing as setup currently in master broken?

yeah, test-browser command was throwing error, also package phantomify isn't updated in last 8 years

Yes, it appears it depends on phantomjs to be installed in a system, which on its own is deprecated

harshzalavadiya · 2021-01-05T06:54:35Z

@medikoo I have resolved conflicts let me know if any changes are required from my end

medikoo

@harshzalavadiya thanks for update!

I've just realized that to not be breaking we need to ensure that package (and tests that confirms that) still works in Node.js v0.6

At least we should not introduce any breaking changes in v0.2.

For further versions we need to at least maintain support for Node.js v0.12, check the roadmap -> #20

This I believe means that we cannot switch test suite to Jest.

I propose to stick to existing test suite, we may just remove browser testing script as I believe it cannot work now as Testling is no longer in service (putting aside deprecations of various packages)

medikoo · 2021-01-05T13:36:04Z

.github/workflows/ci.yml

+    runs-on: ${{ matrix.os }}
+    strategy:
+      matrix:
+        node: ['10.x', '12.x', '14.x']


We need to maintain support for Node.js v0.6 in v0.2 of querystring so this version should be in a Matrix.

harshzalavadiya · 2021-01-05T15:40:05Z

Okay so as you said earlier I'm going to create seprate PR for the things that can be directly accepted and creating issue for test suites and nodejs support so we can discuss it there

harshzalavadiya added 5 commits January 2, 2021 18:08

♻️ changelog + updated readme

e13324a

📝 added jsdoc

f05aee0

✅ moved to jest

b5580fe

👷 added GitHub actions CI

6bae257

🔖 v0.2.1

cdd0b72

medikoo requested changes Jan 4, 2021

View reviewed changes

medikoo assigned harshzalavadiya Jan 4, 2021

harshzalavadiya and others added 4 commits January 4, 2021 11:41

♻️ code cleanup

2b73d3f

🚑 updated references

87bd22c

🏷️ added TypeScript types

91f25aa

Merge branch 'master' into master

5fc0bea

medikoo requested changes Jan 5, 2021

View reviewed changes

harshzalavadiya closed this Jan 5, 2021

This was referenced Jan 5, 2021

proposal for major bump on package v0.3.0 #48

Closed

New versions reflecting Node.js implementation #20

Open

✨ improvements + TypeScript Types #50

Merged

👷 Move to GitHub Actions #49

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package Refactor (Tests, CI) #46

Package Refactor (Tests, CI) #46

harshzalavadiya commented Jan 2, 2021 •

edited

medikoo left a comment

medikoo Jan 4, 2021

harshzalavadiya Jan 4, 2021

medikoo Jan 4, 2021

harshzalavadiya Jan 4, 2021

medikoo Jan 4, 2021

harshzalavadiya Jan 4, 2021 •

edited

medikoo Jan 4, 2021

harshzalavadiya Jan 4, 2021

medikoo Jan 4, 2021

harshzalavadiya Jan 4, 2021 •

edited

medikoo Jan 4, 2021

medikoo Jan 4, 2021

harshzalavadiya Jan 4, 2021

medikoo Jan 4, 2021

harshzalavadiya Jan 4, 2021

medikoo Jan 4, 2021

harshzalavadiya commented Jan 5, 2021

medikoo left a comment

medikoo Jan 5, 2021

harshzalavadiya commented Jan 5, 2021 •

edited


		All notable changes to this project will be documented in this file.

		The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),

Package Refactor (Tests, CI) #46

Package Refactor (Tests, CI) #46

Conversation

harshzalavadiya commented Jan 2, 2021 • edited

Changelog

Tests

Documentation

CI

File movements

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshzalavadiya Jan 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshzalavadiya Jan 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshzalavadiya commented Jan 5, 2021

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshzalavadiya commented Jan 5, 2021 • edited

harshzalavadiya commented Jan 2, 2021 •

edited

harshzalavadiya Jan 4, 2021 •

edited

harshzalavadiya Jan 4, 2021 •

edited

harshzalavadiya commented Jan 5, 2021 •

edited