Skip to content
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

feat: added mocks for browser to fix window, document, HTMLElement an… #180

Merged
merged 2 commits into from
Apr 3, 2020
Merged

feat: added mocks for browser to fix window, document, HTMLElement an… #180

merged 2 commits into from
Apr 3, 2020

Conversation

nickyhaze
Copy link
Contributor

…d navigator issues on server

PR Type

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no API changes)
[ ] Build-related changes
[ ] CI-related changes
[ ] Documentation content changes
[ ] Application / infrastructure changes
[x] Other: Server related change

What Is the Current Behavior?

We sometimes use window and prevent issues by using 'isPlatformBrowser'. This works fine, but unfortunately we sometimes face issues with third party dependencies (AgmMarkerCluster for example), becaude they use window without checking if it's defined.

What Is the New Behavior?

With these browser mocks we prevent any issues regarding window, document, HTMLElement and navigator.

Does this PR Introduce a Breaking Change?

[ ] Yes
[x] No

Other Information

@dhhyi dhhyi self-requested a review April 2, 2020 11:00
@dhhyi
Copy link
Collaborator

dhhyi commented Apr 3, 2020

@nickyhaze Thank you for your contribution. 👍

In the best case scenario, we can always use Angular specific adaptions of libraries that are hopefully SSR-ready. I have seen it myself in projects that this is not always the case. However, for the base project, we should strongly stick to the clean approach, so customer projects have the best solution available for customization. Long story short: I would rather see this feature not activated by default. 😅

Would you be fine with the addition of a commented-out version?

@dhhyi
Copy link
Collaborator

dhhyi commented Apr 3, 2020

Would you be fine with the addition of a commented-out version?

sth like this:

// uncomment this block to prevent ssr issues with third-party libraries regarding window, document, HTMLElement and navigator
// tslint:disable-next-line: no-commented-out-code
/*
const domino = require('domino');
const template = fs.readFileSync(join(DIST_FOLDER, 'browser', 'index.html')).toString();
const win = domino.createWindow(template);

// tslint:disable:no-string-literal
global['window'] = win;
global['document'] = win.document;
global['HTMLElement'] = win.HTMLElement;
global['navigator'] = win.navigator;
// tslint:enable:no-string-literal
*/

@nickyhaze
Copy link
Contributor Author

Perfect, sounds like a good solution to me!

@dhhyi dhhyi merged commit e9d4551 into intershop:develop Apr 3, 2020
@dhhyi dhhyi added the enhancement Enhancement to an existing feature label Apr 3, 2020
@dhhyi dhhyi added this to the 0.19 milestone Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants