simplajs / simpla

Open, modular, and serverless content management for a modern web
https://www.simplajs.org
MIT License
527 stars 36 forks source link

Admin cleanups #62

Closed madeleineostoja closed 7 years ago

madeleineostoja commented 7 years ago

Various minor admin-y cleanups for the repo - @bedeoverend can you confirm I haven't done anything silly? Not overly familiar with our tooling around v2.

madeleineostoja commented 7 years ago

What's that? ~30% reduction in bundle size? Ah yea.

2.0-preview:

2.0-admin:

Would be down to ~ 7kb if it wasn't for IE11 😒 (Promise, fetch, and Object.assign now all supported in evergreens).

madeleineostoja commented 7 years ago

Also saw a whole bunch of references to uid while I was in there (including error messages...). Shouldn't these be stripped out / replaced with path for v2?

madeleineostoja commented 7 years ago

So, uh, what do you think about distributing a simpla-lite.js that doesn't bundle polyfills? Re: #37

(other than object.assign transform in babel, because it's teenier that any of the polyfills for mainline, and is basically syntax sugar so should be part of transform).

If we don't like it, can always roll that commit back :-) Just got a bit carried away...

madeleineostoja commented 7 years ago

See also https://github.com/simplaio/simpla/issues/63

Thought of that because, if we do keep simpla-lite, and both .js and .min.js builds, then the root of this project is getting pretty messy.

madeleineostoja commented 7 years ago

Hmm worth noting that even libs that 'don't bundle polyfills', like SkateJS, bundle minimal versions of method shims like Object.assign, symbols, etc.

Maybe simpla-lite just confuses things? Only use-case for it is to shave off a few kb if you're already polyfilling fetch and promise on your site. Not like someone outright isn't going to support IE11.

madeleineostoja commented 7 years ago

Actually thinking more about it - if we do include simpla-lite, then it should only unbundle the Promise polyfill. Because a) that's by far the most sizeable polyfill we bundle (~2.5k vs < 500b for others), and more importantly b) Object.assign and Fetch are implementation details, we could conceivably build Simpla without them, whereas Promise is fundamental to the API design because it's what all of Simpla's methods return, so it makes sense to expose this requirement to users and allow them to polyfill it on their own if they want.

Anyway, FYI simpla-lite.min.js:

bedeoverend commented 7 years ago

Re: UID - yeah should strip it out, warnings probably more so. But it's just a timing thing: don't think we should bother with stripping it now given it's not breaking - as long as the UID stuff is just internal.

madeleineostoja commented 7 years ago

Also no comment on simpla-lite?

bedeoverend commented 7 years ago

simpla-lite I'm very up in the air on this one too. It might actually be worth doing the reverse. So by default it doesn't pull in deps, but you can ask for one with deps. e.g. how skatejs does it. Basically in the snippet installs, use unpkg with full deps, then if they're using it as a module, we can pretty safely assume they can bring their own heavy polyfills.

But, in general, I think it's important to have files both with and without, it's just which takes primacy. Also I agree re: including unfetch and assign, both of those are minor and don't add too much of a hit. Promises are the nasty ones. Also it's very common for libs to ask users to BYO promises.

madeleineostoja commented 7 years ago

Skate with deps pulls in all the WC stuff, not periphery things like promises AFAIK, which is a different kettle of fish (that is, we don't have any option for including that stuff anymore).

I think it is more confusing for the majority case to have a special file. There's nothing bad that happens if they include a promise polyfill when they already have one, just the extra weight, so it's a performance optimisation. It's also one of the smallest optimisations they can make (eg: HTTP/2, async imports, deduping imports when you need them, etc), so think it's fine for it to be the non-standard import.

bedeoverend commented 7 years ago

Haha, yeah I hear dev's don't really care much about file size 😉 I just think if they're bundling it as a module, the chances of them bringing in their own Promise polyfill is pretty damn high.

I don't think SkateJS (core) has any promises, probably why doesn't use them. Though this is very interesting: https://github.com/skatejs/web-components/blob/master/src/index.js note all the Polyfills to get the webcomponents polyfills to work...

bedeoverend commented 7 years ago

Oh and see my other comment on unfetch, also use it in ping file.

bedeoverend commented 7 years ago

Of course, derp. So WebComponents v1 will require a Promise environment too. Don't know if v0 requires it. It'd be certainly odd to say they need to import our library before WebComponents polyfills so that they get the promise polyfill embedded in ours. Seems weird, don't know practically exactly how that'd go down though.

madeleineostoja commented 7 years ago

Okay just duped fetch import in ping, rollup can handle deduping those yeah?

bedeoverend commented 7 years ago

Yup rollup will deal with it

madeleineostoja commented 7 years ago

From the very top of the v1 webcomponents readme

For browsers that need it, there are also some minor polyfills included:

... Promise

madeleineostoja commented 7 years ago

So, can we rely on users including web components polyfill before promises are used by Simpla? In elements yes, because wherever promises don't work wc don't work, so elements can't call polyfilled promises before being polyfilled themselves. Edge case is user usage of the SDK. I'd say we still bundle it, too big a risk not to.

bedeoverend commented 7 years ago

Haha - my bad, I was looking at the wrong repo, looking at custom-elements. Anyway, as I said earlier, this doesn't bother me that much so I'm happy to include a simpla-lite in the builds.

madeleineostoja commented 7 years ago

Also looking at web-components-loader, I think that will only work if installed locally with bower. For our stuff we'll have to just dump people with the whole web-components-lite 🙄

bedeoverend commented 7 years ago

I didn't mean that we should rely on them importing the web-components-polyfill so that we didn't have to. I was just using it as an example of something else that asks for the Promise polyfill. But as you say, their standard build webcomponentsjs/webcomponents-loader pulls in all the polyfills necessary.

madeleineostoja commented 7 years ago

Ke? I don't understand what you mean - we're getting them to import the web components polyfill still?

What I mean is that by looking at the loader, looks like that only works if they install the polyfill with Bower, since it just creates script tags in their head to other files it assumes they have locally. To link in from unpkg they'd have to pull in the whole webcomponents-lite package.

bedeoverend commented 7 years ago

Oh - that issue is separate to this issue yeah?

Not sure how they do it, if they're working off relative paths, then unpkg might still work as it's just relative URLs, but would have ot experiment, I really don't know.

madeleineostoja commented 7 years ago

Okay bunch of random tests failing on Travis, no idea why - @bedeoverend could you have a quick look when you get a moment?

bedeoverend commented 7 years ago

Yeah I can take a look over in the morning. I'd say it's just unfetch and fetchMock not playing nicely, but I'll have a dig around and see what's up. If fetch native is available then that theory is out the window anyway

bedeoverend commented 7 years ago

WHich it is, in chrome. So something else is definitely off

madeleineostoja commented 7 years ago

I just realised something, and I'm going to completely reverse my position on the Promise polyfill and simpla-lite in general - we shouldn't include a Promise polyfill at all, the default and only bundle of Simpla should only run in environments with Promise provided.

Here's why: If Simpla was just the JavaScript SDK then I would be totally on board with asking devs to make sure they BYO promises. The entire interface is literally promises, so they have to know what they are and it's reasonable to ask them to BYO polyfill for IE, even give examples of linking one in if need be. But Simpla is not the SDK. The majority of users would consume the elements directly, and have no knowledge of the SDK. It's not fair to get these guys to dick around with a Promise polyfill because they're not exposed to it, and they could be novices that don't even know what a Promise is, let alone why they have to 'polyfill' it.

BUT, it's impossible for an element to use an unsupported Promise, because the wc polyfills need (and provide) Promises just to upgrade the element, and everywhere Promises aren't supported (IE11), WC are polyfilled too. So, the only time Simpla would need to explicitly provide its own Promise polyfill would be for users of the raw SDK. And, as above, I think it is totally fine to point out to these guys the req for Promises. We just document that it needs it, explain that the WC polyfill brings it in, and tell them they either need to make sure that's brought in before their SDK calls, or that they BYO polyfill for IE11 (in which case WC won't bring in its promise polyfill).

So, TL;DR: ditch simpla-lite, remove Promise polyfill from Simpla entirely, document situation in SDK docs (not main installation/getting started docs, since wc takes care of that use-case for us).

What say you @bedeoverend?

bedeoverend commented 7 years ago

@seaneking very good point. Yeah that all sounds good - lets just document it well and that should suit 99% of people.

bedeoverend commented 7 years ago

So I had to wrap up fetch so that it could be mocked by fetch-mock. It's not that nice, but there's not a lot of options. Basically we need fetch-mock to mock out network requests to the API, and given the way the SDK is structured (doesn't use dependency injection), and we're using unfetch as a ponyfill rather than a global polyfill, then this is the best way to do it.

bedeoverend commented 7 years ago

Oh and re: the above comment https://github.com/simplaio/simpla/pull/62#issuecomment-293054345, so given our elements still use v0, we're telling users to pull in webcomponents-lite.min.js which doesn't include the Promise polyfill. So we can't document telling them they'll get it if they're using the WC polyfills.

madeleineostoja commented 7 years ago

Are you sure web-components-lite doesn't include promise? How do they support IE?

bedeoverend commented 7 years ago

Yeah I did a couple of checks. It doesn't actually need Promises - I'm not sure why v1 needs promises either actually, but v0 doesn't include the polyfill

bedeoverend commented 7 years ago

Ah, v1 at least needs them for

customElements.whenDefined('my-element').then(handleElementDefined)
madeleineostoja commented 7 years ago

Bah, damn. Well, in that case I'd say we include promises and not do simpla-lite, then once we move to webcomponents v1 just remove promises

bedeoverend commented 7 years ago

👍

madeleineostoja commented 7 years ago

FWIW removing promises (and other stuff related to v1 polyfills) will require a major version bump, but it wouldn't be a big breaking change so no migration guide and etc necessary