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

Corrected "TypeError: Cannot assign to read only property", closes #68 #75

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

danielgindi
Copy link
Contributor

No description provided.

@maiermic
Copy link

@danielgindi Thanks for creating a PR. How did you test it? I saw that there is a merged PR in a forked repository: https://github.com/alist-org/jschardet/pull/1. Has this been used for testing?

@danielgindi
Copy link
Contributor Author

I've tested it in a production system with a few inputs that failed due to this error, and got detected correctly with this PR.
What I did is simply correct the code syntax to do what the author seems to have intended.

@maiermic
Copy link

@danielgindi Thanks for your response. How did you include your changed version of jschardet in your project? I installed jschardet using npm in my project, but AFAIK that does not work without a release of this PR.

@aadsm Does an example project (with the fixed version of jschardet) help you to review the PR? Are manual changes to the codebase even the way to go? Or is this codebase still/regularly generated/ported based on the Python codebase?

@danielgindi
Copy link
Contributor Author

@danielgindi Thanks for your response. How did you include your changed version of jschardet in your project? I installed jschardet using npm in my project, but AFAIK that does not work without a release of this PR.

Just put the git url of the PR branch as the package version.

@aadsm
Copy link
Owner

aadsm commented Mar 1, 2022

Let me see if I can check this tomorrow, otherwise on Wednesday where I'll have more time. If I don't please ping me again.

@maiermic
Copy link

maiermic commented Mar 5, 2022

If I don't please ping me again.

@aadsm Friendly reminder. Don't hurry. Take your time. We are all busy 😉 Thanks for creating and maintaining this project ☺️

@aadsm
Copy link
Owner

aadsm commented Mar 14, 2022

@aadsm Does an example project (with the fixed version of jschardet) help you to review the PR? Are manual changes to the codebase even the way to go? Or is this codebase still/regularly generated/ported based on the Python codebase?

If you could add a test here that would be great. Ignore the qunit stuff, that's what I used at the time, but I've just ported them to jest.

I only (manually) converted this from python when I created the project 12 years ago.

@aadsm
Copy link
Owner

aadsm commented Mar 14, 2022

I've just checked the source line with the issue. I guess this is what I get for not using type checkers, although at the time none existed for javascript.
Thank you so much for fixing this! I'm going to merge but could someone still add a unit test pretty please? 😇.

@aadsm aadsm merged commit a1c0607 into aadsm:master Mar 14, 2022
@vibaiher-qatium
Copy link

Hello @aadsm

Do you plan to publish this change in a new version soon?

github-actions bot pushed a commit that referenced this pull request Feb 29, 2024
Changes since 3.1.6:
2698f33 Version 3.1.6 (patch)\nd500c29 Version 3.1.5 (patch)\n1d1ff49 Version 3.1.4 (patch)\n3a2d31b Version 3.1.3 (patch)\n08d9559 Version 3.1.2 (patch)\n2f9d07c Version 3.1.1 (patch)\nef7d0da Version 3.1.0 (minor) \\n \\n Changes since git tag --list v* --sort=-taggerdate | head -1: \\n\n1e8ceb3 3.0.1\n4d45864 Updated browserify and jest versions\n56e10ee Cosmetic changes to the way the percentage is shown in show-size-changes.sh\n7010531 Add script to determine size changes in the distributed files\n120c0e7 npm audit --fix\na1c0607 Merge pull request #75 from danielgindi/fix/string_as_array\n2b9b07d Corrected \"TypeError: Cannot assign to read only property\", closes #68\n5a57c11 Convert the encodings tests to jest\n08e6c81 Add initial set of jest tests\n7897929 Check if given encodings exist against the denormalized list of supported encodings\naf66fa6 Add typescript support to detectEncodings\n9b49243 Add detectEncondings option\ne5945e2 Merge branch master of https://github.com/aadsm/jschardet\n71bcf43 npm audit fix\n094cb6f Merge pull request #71 from bpasero/patch-1\nc089b44 Add `detectAll` to index.d.ts\n0ae9a3c Update package-lock.json\nf71723b Add .npmignore to exclude tests
github-actions bot pushed a commit that referenced this pull request Feb 29, 2024
Changes since 3.1.6:
296ee53 3.2.0
2698f33 Version 3.1.6 (patch)
d500c29 Version 3.1.5 (patch)
1d1ff49 Version 3.1.4 (patch)
3a2d31b Version 3.1.3 (patch)
08d9559 Version 3.1.2 (patch)
2f9d07c Version 3.1.1 (patch)
ef7d0da Version 3.1.0 (minor) \n \n Changes since git tag --list 'v*' --sort=-taggerdate | head -1: \n
1e8ceb3 3.0.1
4d45864 Updated browserify and jest versions
56e10ee Cosmetic changes to the way the percentage is shown in show-size-changes.sh
7010531 Add script to determine size changes in the distributed files
120c0e7 npm audit --fix
a1c0607 Merge pull request #75 from danielgindi/fix/string_as_array
2b9b07d Corrected "TypeError: Cannot assign to read only property", closes #68
5a57c11 Convert the encodings tests to jest
08e6c81 Add initial set of jest tests
7897929 Check if given encodings exist against the denormalized list of supported encodings
af66fa6 Add typescript support to detectEncodings
9b49243 Add detectEncondings option
e5945e2 Merge branch 'master' of https://github.com/aadsm/jschardet
71bcf43 npm audit fix
094cb6f Merge pull request #71 from bpasero/patch-1
c089b44 Add `detectAll` to index.d.ts
0ae9a3c Update package-lock.json
f71723b Add .npmignore to exclude tests
github-actions bot pushed a commit that referenced this pull request Feb 29, 2024
Changes since 3.1.6:
2698f33 Version 3.1.6 (patch)
d500c29 Version 3.1.5 (patch)
1d1ff49 Version 3.1.4 (patch)
3a2d31b Version 3.1.3 (patch)
08d9559 Version 3.1.2 (patch)
2f9d07c Version 3.1.1 (patch)
ef7d0da Version 3.1.0 (minor) \n \n Changes since git tag --list 'v*' --sort=-taggerdate | head -1: \n
1e8ceb3 3.0.1
4d45864 Updated browserify and jest versions
56e10ee Cosmetic changes to the way the percentage is shown in show-size-changes.sh
7010531 Add script to determine size changes in the distributed files
120c0e7 npm audit --fix
a1c0607 Merge pull request #75 from danielgindi/fix/string_as_array
2b9b07d Corrected "TypeError: Cannot assign to read only property", closes #68
5a57c11 Convert the encodings tests to jest
08e6c81 Add initial set of jest tests
7897929 Check if given encodings exist against the denormalized list of supported encodings
af66fa6 Add typescript support to detectEncodings
9b49243 Add detectEncondings option
e5945e2 Merge branch 'master' of https://github.com/aadsm/jschardet
71bcf43 npm audit fix
094cb6f Merge pull request #71 from bpasero/patch-1
c089b44 Add `detectAll` to index.d.ts
0ae9a3c Update package-lock.json
f71723b Add .npmignore to exclude tests
aadsm added a commit that referenced this pull request Mar 19, 2024
Changes since 3.0.0:
f3de69b Updated bundles
25f107a Updated github workflow files
4d45864 Updated browserify and jest versions
56e10ee Cosmetic changes to the way the percentage is shown in show-size-changes.sh
7010531 Add script to determine size changes in the distributed files
120c0e7 npm audit --fix
a1c0607 Merge pull request #75 from danielgindi/fix/string_as_array
2b9b07d Corrected "TypeError: Cannot assign to read only property", closes #68
5a57c11 Convert the encodings tests to jest
08e6c81 Add initial set of jest tests
7897929 Check if given encodings exist against the denormalized list of supported encodings
af66fa6 Add typescript support to detectEncodings
9b49243 Add detectEncondings option
e5945e2 Merge branch 'master' of https://github.com/aadsm/jschardet
71bcf43 npm audit fix
094cb6f Merge pull request #71 from bpasero/patch-1
c089b44 Add `detectAll` to index.d.ts
0ae9a3c Update package-lock.json
f71723b Add .npmignore to exclude tests

Bundle size changes since v3.0.0:
* dist/jschardet.js +3135 (465888 -> 469023)
* dist/jschardet.min.js +3460 +0.01% (335803 -> 339263)
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.

4 participants