Open mattmoreira opened 4 years ago
Hi @mattmoreira, I read your e-mail, thanks for submitting this PR, I will try to have a look at it soon.
The problem with Evaporate's code "style" and architecture is really big and I agree that it would be awesome if we could split it properly, use the latest tooling and proper types and so on. From the quick glance at the changes, I think this auto-refactor will not be enough regarding the major changes. We could however, build on top of that and think properly about the future (where I'd like to finally resolve #437, use TypeScript, properly test everything and plug in CI), give me some time and I'll write up some proper feedback 😉
[Also we'd still need to get access to NPM releases and CI here to do proper job (shout out to @tomsaffell) who has been quiet for some time 😇 ]
Hey @jakubzitny!
So, I've now added an automatic conversion to TypeScript, and also I've applied prettier to the converted code. Unfortunately, I didn't add the types, as this would take an even bigger effort, but we can use the work done on #405, as a good basis.
Also, I included Webpack for bundling and analyzing, and I was finally able to run the tests on the output. After some manual changes, all of them are passing.
The converted code is available under codemod/output.
PS: I sent today an e-mail to @tomsaffell to try to get the credentials that you've mentioned 😄
@mattmoreira Hello, I think you should make your own fork instead of waiting for him.
Did you publish it on npm? you were able to fix the memory leaks?
Regarding types, you should try TypeWiz.
I'm willing to help you if you're up for making your own fork.
Edit: You can try to contact Thomas here.
There's his phone number, not sure if i can post it on github but it's public on crunchbase.
Edit2: You might also give a shot to lebab and resugar to modernize the syntax.
Hi, sorry I did not get to this sooner. I mentioned some ideas about future development here #450.
I created a https://github.com/evaporatejs/EvaporateJS and I'd release it on NPM as evaporatejs
or evaporate3
, what do you guys think?
Before that though, let's proceed with this TS refactor, make sure everything works as it should, and propose several next steps.
The memory issues are probably not fixed @mtltechtemp, but with new codebase, modules and types, it should be easier to reproduce and test.
Would you be able to build/release a beta version of this, so I can try to upgrade it in my projects to see how "mature" it is @mattmoreira? Could you also propose how the converted code would replace the current one?
Hi, @mtltechtemp,
I think you should make your own fork instead of waiting for him.
I'd rather wait for Jakub's response, as he has been maintaining this repo for a while, I don't think that it would be right to push for this change against his will.
Did you publish it on npm?
No, still, if you want to use the refactored code, you can point npm package to use the address of the new repo, and then you'll be able to use the newest code. I must warn that it is quite unstable, but if you want, I can create a tag, and then you can point the tag on your package.json
Read more on.: https://docs.npmjs.com/files/package.json#git-urls-as-dependencies
Regarding types, you should try TypeWiz.
Regarding types and syntax, thank you for the suggestions, I especially liked TypeWiz, and I'll definitely. use it.
You might also give a shot to lebab and resugar to modernize the syntax
Lebab, I've already applied to the project and it helped a lot, as you can see here.
You can try to contact Thomas here.
Thanks for your help. I've contacted Tom, and we're talking about the credentials, I believe this shall be solved in a matter of days.
You were able to fix the memory leaks?
Nop, I saw issue #437, I tried to investigate it, but I was having some troubles trying to debug it without sending the file to S3. I think that I'll be able to test it by intercepting the request, or something like this. I'll give more details on the issue thread.
The memory issues are probably not fixed @mtltechtemp, but with new codebase, modules and types, it should be easier to reproduce and test.
That's exactly my train of thought as well, @jakubzitny. 😄
Would you be able to build/release a beta version of this, so I can try to upgrade it in my projects to see how "mature" it is @mattmoreira?
Sure! I'll create a release candidate pointing to the build.js, and I'll also add a prepublish script that builds this bundle, ok?
Could you also propose how the converted code would replace the current one?
Good question. I don't know, we can maybe do like storybook does, by using a branch named next, doing the release, testing, and if it succeeds, we rename the next turns into the master, and the master is deprecated. As it's tagged, it will always be there available, so in case something goes wrong, we just revert master to the commit of this tag.
Done @jakubzitny and @mtltechtemp. Release candidate 3.0.0-rc was created 😄
Thanks a lot @mattmoreira, let me test it a bit 🙂
Thank you for doing what you can to continue this project because it is vital to a couple sites I am developing. I really appreciate your work. Maybe in the future I can help contribute, but I would have to learn a few things first to be able to help.
Hey @jakubzitny, can you give us some feedback? Do you think it's stable enough? Should I move the outputted code into the src folder?
@mattmoreira I'm using it right now in my project and it works very well!
Upload speed seems faster but the more impressive is the memory usage. (Makes sense at least because now the variables are scoped.)
Maybe the memory leaks went away.
@mattmoreira Did @tomsaffell gave you the credentials?
@mattmoreira I'm using it right now in my project and it works very well!
Upload speed seems faster but the more impressive is the memory usage. (Makes sense at least because now the variables are scoped.)
Maybe the memory leaks went away.
Very awesome! Maybe that's why I couldn't detect anything when trying to debug #437, as I was using this new TS version.
You might want to add
Prettier
to the project to normalize the code syntax.Don't forget that you could take advantage of TypeWiz for automated types.
Added prettier, works over types using TypeWiz have started on https://github.com/mattmoreira/EvaporateJS/tree/chore/apply-types
@mattmoreira Ok i'm on it too, lemme know if you need help on something. Do you have Discord?
edit: TypeWiz has done something but the types names are horrible, we need to change that.
@mtltechtemp I think I was finally able to make it work! I deleted the old code and checked out the code from the master branch, and made the changes only on the bundle. I think it will work!
My Discord is MattElias#0591
@mattmoreira I'm still waiting for you to accept my request on discord.
Hey @jakubzitny, @TheWiseNoob and @mtltechtemp, I'm back!
Could you please review my PR for applying types to the project? https://github.com/mattmoreira/EvaporateJS/pull/1
I've fixed the previous type names and applied a bunch of new types and interfaces as well.
Thanks for reviewing @mtltechtemp! I'll do some final changes now and release another release candidate with the final code structure.
@jakubzitny, @TheWiseNoob and @mtltechtemp, I've added a PR to refactor the code structure, deleting the old evaporate file.
Can you please review it?
@jakubzitny, I've updated the upstream PR.
@mattmoreira Works great for me! It's been in production for about two weeks and no users are complaining about it.
Good work ! 👍
Edit: Safari, Firefox, Chrome are fine.
Hey guys, I tried to install the latest rc
"evaporate": "github:TTLabs/EvaporateJS#pull/448/head",
in package.json, but when I try to import it, I get the following webpack error: Module not found: Error: Can't resolve 'evaporate' in <file>.js
. Any idea?
Hey guys, I tried to install the latest rc
"evaporate": "github:TTLabs/EvaporateJS#pull/448/head",
in package.json, but when I try to import it, I get the following webpack error:Module not found: Error: Can't resolve 'evaporate' in <file>.js
. Any idea?
I'm having the same issue in a typescript project, how were you able to fix the issue ? @pyr0hu
Thanks in advance.
@camineroirving Sorry mate, could not resolve this so I just reverted to using the latest 2.1.4 release.
Is this still being developed?
Hi, I am also interested in the typescript version of this library. Having the same issue as pyr0hu and camineroirving though, when trying to use it in my project. Is there any plan to continue on this pull-request?
is this still WIP ?
"Evaporate" is not exported by "node_modules/evaporate/evaporate.js" i have this problem
This project has already served me a lot, and because of that, I wanted to somehow contribute to it, especially regarding its maintainability.
Given this, I've decided to develop an automated refactoring of the project, achieved by the usage of an AST parser called Recast, through which I could extract most of the root level classes and utility functions into different files.
I still have a problem with classes being declared and used in the same file, and usages of this before calling super, but other than that, it's almost done.
I've run the automated tests, and after those manual fixes cited above, it started working.
@jakubzitny, what do you think?