-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
ES6 & rollup #2691
Comments
Nope. I've been thinking along the same lines for a while. What about using TypeScript? It might make development / contributions easier to have code hinting / real object linking. Although I don't know if this would put a knowledge burden / barrier then. I was also thinking along the same lines that we should consider using es6 collections for parsing / AST and then using a polyfill for es5 builds like https://github.com/Benvie/harmony-collections. In es6 environments (which Node.js now is, as are latest browsers) that should theoretically speed up parsing / evaluation because of the reduced memory overhead and faster AST construction. |
We can consider that after ES6 - es6 doesn't preclude typescript since typescript is an es6 transpiler too.. it would be an additional step after this. I personally wouldn't recommend typescript unless there was going to be significant extra work or refactoring in less. IMO typescript
I wouldn't use that polyfill, the project looks dead (perhaps also because the name harmony is dead). Also I unfortunately wouldn't be sure that the ES6 native equivalents are faster - https://jsperf.com/property-access-object-array-map-weakmap/6. Its sad how for instance the Promise polyfill is faster than the native implementation :( |
👍 |
All good points. Standardizing on ES6 / Babel often makes things not only easier to write, but read, especially when doing prototypical inheritance. As far as that jsperf test, it's not really a fair example. I'd expect Weakmap to perform slower for that specific test. In theory, it might be better for Less in creating a bunch of different object shapes and types and then evaluating them. Not sure though. I found a few good libraries that I want to eventually test (and hopefully add) where we can hook into individual functions to do granular benchmarking, without having to put any "benchmarking hooks" into the lib. I'm hoping to set something up (when I have time, who knows when that will be) so that you can do a kind of jsperf test on functions within Less.js, but with local development / branch changes to Less vs. the current release, and using a few kinds of test .less files. As I've mentioned before, we shouldn't need to guess which is faster for our particular use case. We could theoretically sub in a WeakMap at some function level vs. raw objects, run benchmarks on that location, and decide based on outcome. |
And one unmentioned, very significant benefit you are skipping over: Moving to TypeScript would probably contribute more than moving to ES6, imho. |
By typed mode you mean the forbid Any?
I tried that on a small project and I had annotations everywhere.. it ends
up with alot of code because the transpiler isn't super clever.
as to how many variables have mutated variable types, im not sure. so..
not convinced the performance benefit would be noticeable.
|
@rjgotten I haven't used TypeScript but those are good points. If you are using an intellisense/autocomplete environment for TypeScript, it might actually help new contributors since it would complete types / object shapes as you make changes. Properly designed, it could serve as a kind of self-documenting / familiarizing / error-preventing tool to someone contributing code changes. But the key there is probably in the TypeScript design. Overall, would there be a performance benefit? That's harder to determine. That's more the artist than the canvas. Even though I'm aiming to use TypeScript for my own projects, I'm probably a little more inclined to agree with @lukeapage, primarily because, unless any of us here are TypeScript experts, I'm not sure we could make effective use of it initially. Also, moving to ES6 doesn't prevent using TypeScript in the future. Microsoft / Google's goal is to make TypeScript 2.0 an ES6 superset, so we could always start with ES6 / Babel, and later add proper types and switch to the TypeScript transpiler. My suspicion is that TypeScript is going to "win" the transpiled languages, just because the compile-time / autocomplete benefits are so much greater over non-typed JavaScript. So... I guess that means that I think using TypeScript is probably inevitable, but that doesn't mean we have to use it right away. |
I would like to discuss a different architectural approach to Less at some point, but that's more of a tangent discussion to this. |
No. Just using typed variables in general. You can forbid use of Any in general, but it's (atleast practically) a bad idea because it tends to lead to lots of code bloat to just satisfy the typing system. However, the use of typed parameters on a function does mean that callers (atleast callers written in TypeScript) must respect the type given. That means typed functions can be used to protect type stability on the inner workings of the Less compiler where appropriate and guarantee that particularly important functions will be well optimized. (Don't forget; JS engines generally optimize at the function level...) |
work started - https://github.com/less/less.js/tree/rollup |
Pretty cool work. Germaine to the discussion, I was looking at the code and quickly came across:
AFAIK that type of sanity check would never be necessary in TypeScript. If the index type is being converted at run-time to string every time to do a string comparison in order to check the type of value, things like that can add up. In TypeScript, if there was a call to a function that passed an index, but that type was sometimes not a number, and index only accepted a number, then the function would fail at compile time because of mis-matched types, rather than needing to be tested at run-time, when such checks are more expensive. On the other hand, it could be that this is an isolated and rare example; it's just something I came across that seemed relevant. |
I think that the biggest advantage of explicit types is that it allows more powerful tooling. Finding all (and only) places that call a function or override it is just a keyboard shortcut away. Finding out which functions the objects you have available is zero work and refactoring is safer because compiler generates more errors. It makes it also easier to learn new codebase - getting big picture, finding out which part of code depends or follow code flow without running it is harder in javascript then in java (for example), because typed editors are more powerful. I did not used TypeScript yet, so I do not know whether tools are mature enough to make it worth it. Advantage of javascript is that everyone knows it, TypeScript is less known. Overall, I think that move to TypeScript would make sense if we planned some kind of bigger refactoring or huge new features, but it might be too much work while we are just fixing small bugs. |
This may be the case, and really, to be pragmatic, @lukeapage has already taken on the work so I'm inclined to say he should make the final call at this time. :-) |
Hey, about this: is there anything we should do to let people know that the codebase is changing? I assume that otherwise you'll have merge conflicts. |
no, shouldn't be too many conflicts. first step is just es6 modules - not
moving any files, then we can add babel or typescript transparently.
|
Hi, guys, how's going now? |
I can work on this. @matthew-dean Would you accept a pull request? |
@alexlur For sure! My only concern is the refactoring for the There would also be a tiny bit of re-writing of the Gruntfile so that tests are using builds and not |
@matthew-dean Hello there. I am done with the refactoring but I don’t know how to configure Gruntfile to run tests on the new built files. |
Maybe what I can do is merge your changes into a separate branch and see about integrating tests. I was just quickly looking over your changes. Correct me if I'm wrong, but is |
Good catch, I used the wrong file. |
Is there a minimum version of browser / Node.js that less.js must support? Node.js has fairly good support for Edit:
Only Internet Explorer 11 doesn’t have |
@alexlur This way users of modern browsers as well as users that have Promise globally polyfilled before Less is loaded, will have everything magically working. And users that don't want to -- or for whatever reason are not allowed to -- globally polyfill can make things work with one tiny additional step. They need only manually assign their chosen Promise implementation to |
@rjgotten @alexlur Less.js already has less.js/lib/less/import-manager.js Line 5 in 55380d4
On the Node side, there's this issue still open: #3121 However, without any feedback, and with a fairly moderate amount of contributors, I'm inclined to just say Node 4+ is the way to go for now. We should update docs / tests to reflect that. |
@matthew-dean It should be ready to be merged to a separate branch. |
@alexlur Thanks! I will try to take a look soon. Will probably be next week (unless someone else has the slack to check it out). |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@matthew-dean I’ll limit myself to just converting to ES6 and leave the TypeScript part later. |
@glixlur I think ES6 is probably the most "community-friendly" first step moving forward, so that's perfect. I'm not sure everyone is on board with TypeScript, but definitely an ES6 flow would be great. |
@matthew-dean I assume the lowest supported platforms are Node 6 and IE11? |
@glixlur Appveyor is currently set up to test Node 4. Would that make things more difficult? Ideally, it would be a Babel-transforms a |
Done, but needs to get the tests working. PhantomJS is deprecated and has no support for anything after ES5. |
@glixlur Nice! Re: PhantomJS, it really needs to be replaced by something like Chromy. See: #3240 |
@glixlur Also, this is Chromy / headless Chrome related: #3262 |
@glixlur Regarding PhantomJS - those tests are running against the bundled browser version, which you would presumably be transpiling into ES5, correct? Why wouldn't PhantomJS work with the ES5 output bundle? |
@matthew-dean Yes, but there is a bug where |
@glixlur You need the |
@matthew-dean |
@glixlur Look into Also, iirc the Babel team is working on limiting the polyfills bundled by |
@glixlur @rjgotten Ah. I should have clarified that I don't know exactly which solution to use re: Babel. Just that it's because |
It will indeed. |
Should phantomjs be deprecated? |
Considering it's dead? |
@glixlur See: #3240 |
Conversion done and merged! See: less/less-meta#32 |
Welp, this week I learned that |
Ironically, I ran across the very same thing with another bit of code not related to Less, something like 2 weeks ago and I distinctly recall the thought hitting me:
Well: question answered, I guess? I so do not envy the storm of users that poured their discontent out over your work, @matthew-dean. You have my sympathy having to handle that situation. 😄 |
@rjgotten lol I mean, how could you anticipate something like that? The safest thing would have been to not convert at all, or to never use classes, but it makes the codebase less verbose (well, or potentially will, there's a lot of opportunity for further clean-up). Technically, the (historical) Like, if someone hacks the prototypes on your library, how do you code defensively for that?? 🤷♂ |
Technically, I released two beta versions after the conversion, BUT... no one has Less betas in their pipeline. So there's no way to test 800,000 repos that integrate with Less and run their tests. With NPM dependencies, you really have to just put it out there and see what breaks. One thing we could do in the future is add some larger Less dependents to tests, if we can figure out what they are? The other thing I could have done is released it as a major version, but.... it technically DOESN'T have any (intentional) breaking changes, i.e. the supported features are identical, and it includes some bug fixes, so.... I dunno, this was a tricky one. |
Yeah. It was a tricky one. I guess you could've tried to just put out something on a Either way, major version or not, you'd probably end up with being forced to just "give it a go" and see what breaks. Don't get me wrong though: the refactor is a very good thing. Moving forward it should generally take a good slice out of the maintenance burden.
You literally don't. It's like people complaining that their carefully orchestrated runtime-emitted IL to access private members in C# breaks across minor releases. If you meddle with internals, you should know what you're buying into. 😄 |
@rjgotten Speaking of refactoring... ..... One of the thing I've discovered over time in the Less codebase is that there are a number of places where there are slight / subtle mutations on objects in the AST. A new property here / an added method on an instance there, and I'll fully admit to having done this at times with features I've merged in, because in JavaScript, you can mutate whatever you want, whenever you want, whether you documented/defined that property or not. There was some discussion about whether or not TypeScript vs. just modern JS would be the answer. I've realized that not having types or clear object interfaces makes the Less codebase at times very hard to reason about in a way that no amount of documentation could solve or even JSDocs could solve. I set up the new pipeline to allow TypeScript linting if you have proper types in JSDoc, but using JSDoc for typing is waaaaaaaay more verbose / visually noisy than TypeScript, and there are some typing features that TS just doesn't support via JSDoc annotations alone. There are no JSDoc / ESLint combination of tools that can solve some of these things that TS gives you for free. So, I guess all I'm saying is that after using TypeScript significantly in the last year, and having spent years in the Less codebase, I'd say 95% of the confusion/frustration/learning curve of figuring out how Less works would have been avoided if objects had compile-time types enforced. I've often spent a lot of time in a debugger just setting breakpoints and figuring out what an object's properties actually are, instead of how it's defined in a file. For instance, there are a number of features that Less has that, during evaluation, rely on a In TypeScript (with common tsconfig settings), this wouldn't be allowed. It wouldn't even compile. You would be forced to specify the shape of So, I guess at this point, I've gone from thinking, "TypeScript is something to be considered" to feeling like, "If your public project isn't in TypeScript, you have a large technical debt." I would never start any open source repo without it, because it inherently helps solve consume-ability and maintainability. You still need clear documentation, but at least it enforces extremely clear and cohesive coding. That's just me thinking about where to go from here, and ways to prevent pain points in the future. (Addendum: in case it's not clear, I'm 100% not bashing anyone's historical work in the Less codebase. There are a tons of things that I've proposed, got buy-in, and merged into the codebase that now I'm like, "Oof, I wish I hadn't done that," or wished I'd done it differently. And I've certainly added things that I realize now might have negatively impacted maintainability. Everyone's done their best.) |
Actually; I ran across the issue with JSDoc support being so-so for type inference myself as well. It used to be a lot harder to work with declaration files, but modern versions of TypeScript's compiler will automatically associate a That might be what you're looking for. |
@rjgotten Can you explain a bit more (or link if you know a good resource), of how this might work well with Less's codebase? As in, how do you do this on a file by file level? Is there a What are the advantages of having a |
@rjgotten I was talking to the developer of Chevrotain today, and he says what he does is develop in TS BUT he actually manages an Then he uses a test with an assertion that the internal types and the public API types match -> https://github.com/SAP/chevrotain/blob/master/packages/chevrotain/test_integration/definitions/api_type_checking.ts#L1-L2 |
Chevrotain's way is a good way of enforcing your public API doesn't accidentally change. Very robust, if you think you need that level of protection. But also incurs some overhead. Regarding the use of If they go side-by-side, then any IDE which is backed by the TypeScript compiler for auto-suggestions, linting, etc. will automatically pick up on the typings. Afaik also if those scripts with side-by-side typings are imported from You can also put typings into a single (set of) /**
* @typedef {import("../api.d.ts").MyType } MyType
*/ Using a single bundled typings file is a good idea if you have to take your package through a transpiling step before distribution. In such a case what users will actually |
I'd like to (if I ever get time)
It would be a new major release and I'd also remove the browser bundled promises as I've mentioned in other issues.
Any objections before I just do it? I'm not going to start converting everything to es6 other than the requires (for rollup) but other people are free to use less.js "upgrading" as a way of learning es6 if they like.
Update by @matthew-dean -- I ended up keeping
lib/
in the same folder to keep (some?) of git history, instead of transpiling (up-transpiling?) and moving at the same time. The modules don't compile toes5/
either or the equivalent. Instead, there's a CommonJS single-file build for Node 6+, as well as a browser build.The text was updated successfully, but these errors were encountered: