Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upVersion compatibility issues #90
Comments
This comment has been minimized.
This comment has been minimized.
|
I'm going to need some more info here, I don't quite follow. Where is the error/breakage in the Fiddle you linked to? |
This comment has been minimized.
This comment has been minimized.
priyakanth024
commented
Aug 8, 2016
|
Thanks for the response. So this is the right rendering of the elements on the page: https://jsfiddle.net/hyk3k554/ - this uses fastdom 0.8.6 as an external resource But here is where it breaks when the latest fastdom (1.0.3) is used: https://jsfiddle.net/10hn9hd6/ The whole reason for this is widgets.js which Twitter uses to render the widgets use 0.8.6. Since |
This comment has been minimized.
This comment has been minimized.
|
Ah OK I see the issue now. Possible quick fix:
If this hack works out, we could look at writing something similar into the core. |
This comment has been minimized.
This comment has been minimized.
indianburger
commented
Aug 8, 2016
|
Hello, I work with Priya and I can add more context. We are from Twitter and the maintainers of widgets.js, not the host page. We are third party javascript on a page, we have no control on what version of fastdom is run in the host page. We bundle fastdom in widgets.js (with npm and webpack as a require('fastdom')). When we do a I do not know the history of why you check for window.fastdom before injecting it so apologies if I'm oversimplifying: perhaps you can check if it's a common-js environment, and drop the check? |
This comment has been minimized.
This comment has been minimized.
indianburger
commented
Aug 8, 2016
|
Oh, and I'm happy to submit a PR if that's something you are willing to accept. |
This comment has been minimized.
This comment has been minimized.
|
Thanks for your help on this guys, this is a really interesting bug :) We cannot simply drop the check and allow I think there are two cases we need to support: A. New version of fastdom loaded, then old version of fastdom loaded via The first fastdom loaded always wins. I think the best solution is:
This means that if Does this sound sensible? |
This comment has been minimized.
This comment has been minimized.
indianburger
commented
Aug 9, 2016
|
There are a few downsides with this:
I agree running 2 instances of fastdom is suboptimal but it's not the worst either. It is better than not using fastdom at all, and people could always implement their own version of layout batching which becomes less optimal than a single instance of fastdom. 2 instances is still better than none because each of the instance is batching reads and writes and collisions are an edge case right? Correct me if my assumptions are wrong here. Considering that, would you let 2 instances of fastdom run in a commonjs/amd case? |
This comment has been minimized.
This comment has been minimized.
|
We did use to do something similar in I believe having more than one instance of Let's say you had two UI components, each with their own version of Component A:
Component B:
Each component's The more Does that make any more sense? Sorry if I'm not too good at explaining. |
This comment has been minimized.
This comment has been minimized.
indianburger
commented
Aug 10, 2016
|
First, thanks for being so responsive. I think I understand what you say and I'll try to make my case, hopefully clearer. I agree that having multiple fastdom instances is bad. However, in a large website built with libraries, each library wants to do the right thing and batch reads and writes. And these libraries do not all not use fastdom, which is the reality, and that means inevitably there will be layouts thrashing. Unless there's a browser spec to do this natively, there will always be layout thrashing. Also we are talking about an edge case where fastdom is included multiple times and in this edge case I argue that multiple fastdom instances is better than the downsides. The downsides are that you are breaking versioning and always leaking a global. I could be a website owner running latest fastdom but I might be running an old version of fastdom without my knowledge because some library used it. That's my argument for allowing multiple fastdom instances when used in a module system, but there's also a workaround for us without running multiple instances. The more narrower fix for my specific case is to not leak a global, just for commonjs/amd. We are ok to use the fastdom instance that the host page uses even if its older(through your aliasing suggestion), but our main concern is we don't want to leak a global. widgets.js is running on a lot of websites and we try to never pollute their global window. In the end, I can see that it's not by the philosophy of your library to not be optimal about layout especially when that's the point of your library. Perhaps a fork where we don't pollute the namespace is the solution for the |
This comment has been minimized.
This comment has been minimized.
steffenweber
commented
Aug 12, 2016
|
I originally reported this issue a few days ago on the Twitter Developer Forums: JavaScript error in Widgets-JS on websites using FastDOM 1.0 @wilsonpage said one possible solution could be:
That's what I did a few months ago. But this workaround broke 1 week ago when Twitter started using the method
The point is: |
This comment has been minimized.
This comment has been minimized.
|
OK, sounds like this is getting messy. Here are the options I can think of: A. We encapsulate the
B. Libraries (like Twitter's
// consumer code
var fastdom = twitter.fastdom || require('fastdom');C. Libraries which play with the DOM (like twitter.setDomManager({
measure: fastdom.measure.bind(fastdom),
mutate: fastdom.mutate.bind(fastdom),
});D. Fastdom internally checks for pre-existing versions on a more obfuscated
Let's discuss these options. If anyone has any additional idea, please put them forward. |
This comment has been minimized.
This comment has been minimized.
Isinlor
commented
Aug 12, 2016
|
Sorry for jumping in the discussion out of blue, but isn't option C generally accepted as the best practice known as dependency inversion principle? As far as I can see it solves the issue fully without creating any drawbacks form technical point of view. It allows libraries to depend on abstract, self-defined and therefore stable interface. It solves the issue with many instances of fastdom for end user. It even allows to replace fastdom with some other implementation if fastdom for some reason won't be fitting library end user preferences eg. if browser will start to do it natively. IMHO option C should be put in README as a best practice for library creators :) . |
This comment has been minimized.
This comment has been minimized.
domenic
commented
Aug 12, 2016
|
@wilsonpage asked me for opinions on the best solution here. This is a tough problem. Most libraries don't have this kind of global coordination problem; the worst thing that happens when multiple instances are used on a page is some code bloat. That means most libraries work well with module system-based isolation. But I can see where fastdom is different. I see a two main paths:
Personally I don't think B or C buys much; they require people to use them correctly, so they are only marginal gains over "do nothing", and not as user-friendly as "try to solve the coordination problem transparently". |
This comment has been minimized.
This comment has been minimized.
|
@domenic thanks for your perspective, mega appreciated! |
This comment has been minimized.
This comment has been minimized.
indianburger
commented
Aug 12, 2016
|
I'm with Domenic, either A or D sounds ok. I think there's also another option E[1] we could explore but I'm not sure how feasible it is. I would go with A because:
I got one suggestion if you go with D. You had a major version upgrade which actually made the api incompatible, and in a future 2.x there could be another breaking change. I would suggest a namespace that accounts for the major version so we know that the api is always backwards compatible. e.g. [1]: Option E: This is based on my rudimentary understanding of Fastdom implementation. Could we make the fastdom module private, but the intenral read and write queues globally shared in window. If the queue data structure is stable across versions, then we can have bug fixes and version upgrades be more meaningful because you'll be running the version you expect but use the global queue for storage. (I'm being super naive about race conflicts, so this option might be infeasible) |
This comment has been minimized.
This comment has been minimized.
I think this is a good solution and is basically what @domenic suggested above in his last few bullets. The downside is that it won't be a backwards compatible change, so consumers on @indianburger would you be happy with this 'shared global singleton piece' being stored/leaked onto a This shared piece would effectively be a global task queue. It would have two lists 'measure' and 'mutate' and would flush on My concern with this approach is that It may be wishful thinking, but I'm not planning on changing the |
This comment has been minimized.
This comment has been minimized.
indianburger
commented
Aug 16, 2016
|
On Tue, Aug 16, 2016, 6:13 AM Wilson Page notifications@github.com wrote:
|
priyakanth024 commentedAug 8, 2016
Hello, using an older version of fastdom (0.8.6) throws an error with
readandwritemethods when overridden by bowser or website's newer version of fastdom - they're replaced withmeasureandmutate. Is there a workaround you'd suggest to make the older version not break?Here's an example: https://jsfiddle.net/10hn9hd6/
Browser used: Chrome 51.0.2704.103 (64-bit)