-
-
Notifications
You must be signed in to change notification settings - Fork 133
Convert Aurelia CLI /lib code to TypeScript
#1211
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for the effort, @nenadvicentic! au1 cli is in maintenance mode. I am not sure do we want to do the ts convertion @bigopon? About the change itself, since the lib/ files are dist files, it can be called dist/. And we don't need the whole lib/ folder committed in git at all, they are output of tsc. That build task can be took care of by setting up "prepare" or "prepublish" npm script. |
|
Hi @3cp, No problem for the effort. Regarding having no prior discussion - I started this in an attempt to understand what is going under the hood of the CLI. And I kept going. It's is completely OK if you think it's better to put this on the shelf. I did my best to make only usefull and low-risk changes, in order to minimise chance of errors/regressions. I agree with you about the test coverage. What you have covered is helpful, but there are many things that are not covered by unit tests. The best test for this version is to use this version on real-life projects. I can do it with 4-5 projects, but admitedly, they all follow similar pattern (CLI + TypeScript + SASS/LESS). In case you guys decide to proceed, I can make suggested change to |
|
@3cp Btw, I noticed that in |
|
The cli and v1 skeleton has not been touched for years. We have to lock gulp to v4, as v5 has many unfixed issues. |
|
I have updated cli package.json. |
I dont think we shoul make change to a in-maintenance repo, except when someone really wishes to do so and willing to prove their determination via their work. If this condition happens, then I think its good to have. But you make the call @3cp . |
|
I think we hold this one for now. Cli is only must have in a cli-bundler based app. For webpack based app (skeleton still use cli as task runner), or dumber based app, cli is irrelevant or can be removed. |
|
@3cp, @bigopon thank you both for taking the time to look into this. I agree with you, it's a pretty big change, let's put the merge on hold and have more time to think about it. I would like just to add some more details. This feature branch I would still like to tidy-up this branch (dist folder, add more simple types) in coming days, even if only for myself. I would also love you to share some more feedback with me, if you have time. Finally, I would encourage you to try it in your own cli based projects: npm install -d "https://github.com/nenadvicentic/cli.git#feature/typescript"From everything I tested to far, this really works. Aurelia CLI in general
I understand your point of view, specially in light of Aurelia v2, but from a perspective of a company that has Aurelia v1 webs in production, I am sure that you guys agree with me, at least partially, because there is an effort to refresh v1 documentation and also recently did #1210 , based on issue I filed: Update package.json dependencies for new projects. So it should not be left to die. All that said - thank you both again for your time and effort you put in Aurelia and in looking into this pull request. |
436529e to
8417e3b
Compare
d2e6d45 to
1e00339
Compare
…e extension to `ts`. Leave bluebird configuration files in `/lib/resources` folder for backward compatibility.
c6cb3b2 to
068e4a8
Compare
…S version set to 14.17, since it's minimum runtime requirement for TypeScript 5.1+.
068e4a8 to
a639ef6
Compare
|
I wanted to update you on the status of this pull request. I've been using this final version of the pull request in multiple Aurelia CLI production projects for about three weeks, and it has proven to be stable, with no further modifications needed. Bellow is the summary of where we ended up. Commit StructureThe PR is now cleanly rebased into two logical commits:
Change BreakdownChanges fall into four clear categories:
Key Improvements
Testing ValidationThe changes have been tested across: Production Environments
CLI Functionality
New Project Templates
Final Outcome
Again, I completely understand if you prefer to keep this pull request on hold, but please, when you have some time, take a look into it. In the meantime I won't be making any further changes on my own. If you have any thoughts, questions or suggestions, do let me know. |
…r is outside of project's root folder (aurelia#1213)
…r is outside of project's root folder (aurelia#1213) + fix: `bundled-source.js` - transformed source map is saved to cache. (aurelia#1216)
Hi,
I converted JavaScript files from
/libfolder to TypeScript. More percisely:/srcfolder andoutDiris/lib.npm run buildornpx tsc) overwrites existing files*.jsfiles in/libfolder.*.jsfiles are still CommonJS modules, backward compatible with previous version./bin/aurelia-cli.js.Added example toReadMe.mdhow to run single Jasmine unit-test (e.g.npx jasmine spec/lib/build/bundled-source.spec.mjs --filter="transform saves cache")My motivation was, since company where I work still has many Aurelia v1 projects, to make the code easier to understand and, perhaps, lower barrier for future contributions.
I tested:
wrapShipforleaflet.drawmodule. I tested the web itself in the browser succesfully.au generate component my-component.This pull request is 3rd attempt upgrade:
*.mtsmodules, but I ran into issue with mocking ofsrc/build/utils.mtsmodule in Jasmine unit tests. Methods on ESM modules cannot be overwritten, like methods on CommonJS modules. There are multiple approaches for mocking of ESM modules in NodeJS, but it seems they have been changing in every version (e.g.--loader,--experimetal-loader, now recommndation is to use--importand*.jsfile that usesmodule.register()to register custom loader hooks).requirecommand changed withimportorawait import. Eventually I rolled back torequiresince I was troubleshooting issue (Cannot find module '@babel/plugin-transform-modules-amd') that ended up being in the filesrc/build/amodro-trace/read/es.ts. Loading of Babel plugin'@babel/plugin-transform-modules-amd'viapluginsoption oftransform(...)method did not work, so plugin is loaded in the header of the file withregistercommand now. I suspect the culprit is that I used symbolic link foraurelia-cli(npm link aurelia-cli). And I switched fromtransformtotransformSync, since former is going to be removed in Babel v8. This was only notable issue I had.Going forward from this version it would be possible to further improve type checking, update more of the old JS syntax, use asynchrounos module loading
await importwhere feasable and so on.Let me know if this is something that would be interesting for you to merge in Aurelia CLI repository and if you would like me to make further changes.
Update:
Error:minificationResult.mapshould be string!when minifying production bundle.ReadMe.mdhow to run single Jasmine unit-test (e.g.npx jasmine spec/lib/build/bundled-source.spec.mjs --filter="transform saves cache")Best,
Nenad