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

Replace Python 2 scripts with NodeJS scripts. #95

Merged
merged 7 commits into from
Jun 27, 2020

Conversation

jchv
Copy link
Contributor

@jchv jchv commented Aug 27, 2019

If this PR is too big, I can redo this as individual PRs per file. They should all work individually.

This PR is related to the plan in #93 and implements the hopefully least controversial initial step. It replaces almost all of the Python 2 scripts with Node.JS scripts. I've tried not to rely on any Node.JS features too new, but since Node.JS 10.x is the current LTS version I think it is reasonable to depend on Node.JS 10.x features, which include useful things like recursive mkdir. As such, I've changed the Node.JS version to 10.x in Travis CI. Node.JS 11.x+ brings useful features like newer ES language additions and functions like Array.prototype.flatMap, so it can slightly improve the quality of the scripts when upgraded.

I also felt it was important to be as minimal as possible with library additions. Many things that would often be shelled out to libraries on NPM are done by hand instead. The one exception is Express, which is used for the devserver. I felt it was too valuable to not use in this case; writing a proper static file server from scratch is far from insurmountable but it seems unnecessary.

There is not a standard format that I can see among either Node.JS nor TypeScript in this project, so I just processed my code with Prettier and converted it to 4 spaces (since everything else uses 4 spaces.) I strongly recommend choosing a format and formatter (does not matter which) and sticking to it. It's just less crap developers have to worry about.

There are two scripts not yet converted:

  • serve_files.py: I think this is used for the remote FS in v2. I will port this, but there's enough going on, including some security bits, that I feel it would be better to wait on it.
  • serve_practice.py: I don't know how to test this yet. It's not very much logic so it shouldn't be too hard if we want, and I plan to take a look at it later.

FWIW: The vendor_build.js and vendor_license.js files should produce identical results and behave extremely similarly to the former Python versions. This I've tested numerous times to be completely sure, but suggestions welcome if anything is off.

@KOLANICH
Copy link

KOLANICH commented Aug 27, 2019

I wonder if it is possible and/or makes sense to replace some dependencies names with links to their git repos? Of course it has a limitation: dependencies of dependencies will likely still be in npm (and we need to do something with it, probably to patch npm). Though I still think that phasing npm packages out is beneficial (because code on GH is the code contributors work on, though it often doesn't help, for example an explicit eval backdoor in pybitmessage was present for quite a long time in the code on GitHub and noone (except the attackers) has noticed it untill exploitation in the wild has started), so even such limited phaseout should be beneficial.

@jchv
Copy link
Contributor Author

jchv commented Aug 27, 2019

Interesting thought. With the current build system setup, this would be challenging; packages pushed to NPM are generally precompiled to the dist folder, which is what Kaitai WebIDE actually uses. If you pull from Git you won’t get the dist files. Maybe you could work around this by building the projects, but this requires pulling dev dependencies and sometimes may even require external toolchains to be present. It would certainly end up increasing the amount of NPM modules needed.

Ultimately, I do understand the apprehension towards NPM/NodeJS, but I think that it needs to be balanced with what is good for the health of WebIDE, and I think aligning WebIDE closer to common best practices rather than further from them will be worth it.

Regardless, I’d like to avoid increasing the scope of this PR beyond what it already is; I think a discussion about trying to reduce dependency on NPM would belong in its own issue, probably. If such an issue were created I’d be happy to continue discussing it as a talking point about what kinds of goals WebIDE has as a project and what reducing NPM dependencies is hoped to do for the project (for example, to help improve security prospects.)

build.js Outdated Show resolved Hide resolved
@cherue
Copy link

cherue commented Mar 8, 2020

This is great and should be merged. Python 2 is not supported anymore.

Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

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

@jchv Overall looks good, thanks! Could you just address a few comments below? Otherwise I think I can merge it.

Comment on lines 22 to 24
const files = recursiveFind("formats/", /\.ksy$/).concat(
recursiveFind("samples/", /.+/)
);
Copy link
Member

Choose a reason for hiding this comment

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

This code doesn't convert Windows paths with backslashes (e.g. formats\archive\zip.ksy) to paths with forward slash (e.g. formats/archive/zip.ksy) like the old Python script did (see genKaitaiFsFiles.py:18). The result is that the Web IDE doesn't work when bundled on Windows.

Suggested change
const files = recursiveFind("formats/", /\.ksy$/).concat(
recursiveFind("samples/", /.+/)
);
const files =
recursiveFind("formats/", /\.ksy$/)
.concat(recursiveFind("samples/", /.+/))
.map(path => path.replace(/\\/g,'/'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool thanks, I've added this (but haven't tried testing it on Windows yet.)

Copy link
Member

Choose a reason for hiding this comment

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

but haven't tried testing it on Windows yet.

I have, in fact, I test everything on Windows 😀

build.js Outdated
Comment on lines 56 to 61
return String(d.getUTCFullYear()).padStart(2, 0) + '-'
+ String(d.getUTCMonth() + 1).padStart(2, 0) + '-'
+ String(d.getUTCDate()).padStart(2, 0) + ' '
+ String(d.getUTCHours()).padStart(2, 0) + ':'
+ String(d.getUTCMinutes()).padStart(2, 0) + ':'
+ String(d.getUTCSeconds()).padStart(2, 0);
Copy link
Member

Choose a reason for hiding this comment

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

The second argument padString of the method String.prototype.padStart should be a string, not a number. It works, because JavaScript is dynamically typed and it gets automatically converted, but it's confusing.

Suggested change
return String(d.getUTCFullYear()).padStart(2, 0) + '-'
+ String(d.getUTCMonth() + 1).padStart(2, 0) + '-'
+ String(d.getUTCDate()).padStart(2, 0) + ' '
+ String(d.getUTCHours()).padStart(2, 0) + ':'
+ String(d.getUTCMinutes()).padStart(2, 0) + ':'
+ String(d.getUTCSeconds()).padStart(2, 0);
return String(d.getUTCFullYear()).padStart(2, '0') + '-'
+ String(d.getUTCMonth() + 1).padStart(2, '0') + '-'
+ String(d.getUTCDate()).padStart(2, '0') + ' '
+ String(d.getUTCHours()).padStart(2, '0') + ':'
+ String(d.getUTCMinutes()).padStart(2, '0') + ':'
+ String(d.getUTCSeconds()).padStart(2, '0');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yep, it would be much clearer using strings; this was probably accidental, although I can't remember anymore. Fixed.

@jchv
Copy link
Contributor Author

jchv commented Jun 27, 2020

Thanks for taking a look, I applied your suggestions (and rebased.) I think the mimetype fix that was applied to master also needs to be added to the JS version, so I will also add that on top of this too.

@generalmimon
Copy link
Member

generalmimon commented Jun 27, 2020

I think the mimetype fix that was applied to master also needs to be added to the JS version, so I will also add that on top of this too.

It's possible that it's not necessary, it seems that Express handles the MIME types out of the box. I haven't noticed a problem with the serve.js script, the Web IDE worked normally, but regarding the Python version serve.py I wasn't able to make it work without the declaring the list of extension -> MIME type map (#103).

@jchv
Copy link
Contributor Author

jchv commented Jun 27, 2020

I think the mimetype fix that was applied to master also needs to be added to the JS version, so I will also add that on top of this too.

I don't think it's necessary, it seems that Express handles the MIME types out of the box. I haven't noticed a problem with the serve.js script, the Web IDE worked normally, but regarding the Python version serve.py I wasn't able to make it work without the change introduced in #103.

You're right - the problem I'm having looked like it was related MIME types, rather it's that the build is failing somehow. Hmm.

Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@generalmimon generalmimon merged commit 05c6c85 into kaitai-io:master Jun 27, 2020
generalmimon added a commit that referenced this pull request Feb 13, 2023
See #95 - the
build scripts previously in Python were rewritten to Node.js.
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.

5 participants