mediamonks / muban

A backend-agnostic framework to enhance server-rendered HTML using a modern webpack development setup.
https://mediamonks.github.io/muban/
39 stars 15 forks source link

es6/symbol issue on IE11 #127

Closed melsd closed 4 years ago

melsd commented 4 years ago

Hi there,

We discovered that we are importing: import symbol from 'core-js/es6/symbol'; symbol; on bootstrap.dev.ts file but not on bootstrap.dist.ts and then we get an error because symbol doesn't exist on the build.

I think we should add this import to the dist file to avoid future issues, or is there another way to handle this issue?

Thanks!

ThaNarie commented 4 years ago

Hi @melsd

The reason that it exists on dev by default is because it's needed for other code that only runs in dev mode, but not on production (certain schema validation).

When using symbol in your own code, you should remove the exclusion from the babel config: https://github.com/mediamonks/muban/blob/104230ccc8fef6f8de5f94f92a979ae0f3f99573/babel.config.js#L26

Because symbol is not used that much, and requires some code to polyfill, i made the choice to not include it in the output by default (to save file size).

But I understand that having it included in dev makes it harder to spot any potential IE11 issues.

Happy to hear from others, @skulptur @ThijsTyZ

skulptur commented 4 years ago

@ThaNarie if it is not easy or not feasible to remove the code that uses symbol in dev, then I think we should just add this by default. Same in case we have other dev only polyfills.

Let me know if you want me to work on this.

ThaNarie commented 4 years ago

It is used by html-extract-data (https://github.com/ThaNarie/html-extract-data#production) when not in production (using Joi to validate during dev).

html-extract-data is just a util, used by one of the knockout utils (https://github.com/mediamonks/muban/blob/866dbcb71d27ffc24c5e937bf65681af6b93623c/src/app/muban/knockoutUtils.ts#L2)

In most cases we won't need the symbol (since most projects don't use those knockout utils).

Maybe a better option is to test if Symbol exists in that util function, and throw a super descriptive error on how to enabled it during dev ?

In other cases, you would get the same "symbol missing" errors on dev and prod, so the user can add it themselves.

How does that sound? @skulptur

skulptur commented 4 years ago

I'm wondering if we should have that there in the first place. Now that I know it exists I might consider using it, but at the same time it feels very specific to include by default.

Wouldn't just checking it in that function still leave the door open for us to use symbol or libs that use symbol, and again fail in prod? I would rather not have this type of gotcha.

ThaNarie commented 4 years ago

Sure, the other option is to enabled it in both dev and prod once you need it. Serves a bit more code, but less error prone.

Ideally the Symbol code gets just included without polluting the global namespace, but unfortunately Joi is doesn't support that (as most libraries don't).