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

Monkey-patching window instead of giving a full new instance #4

Merged
merged 2 commits into from
May 8, 2020

Conversation

rgolea
Copy link
Contributor

@rgolea rgolea commented May 5, 2020

Hi there!

I see that this project is a dependency of swiper.js and I had an issue with Ionic because of swiper.js and angular universal. You can check the full issue right here: #21138.

It seems like somewhere along the lines, window is defined for SSR and swiper depends on this repo to get the screen size. Since there might already be a partially injected window, I thought that the best course of action is, instead of using one or the other, it should be monkey-patching the values so any other values can be added to another partial window interface.

I also had to switch to webpack/typescript. Had some issues with the version of gulp that it was used here. This gives also some benefits of having type safety when using the library.

Hope it helps and if this is not the way to fix the issue, please help me into pointing me in the right direction. Thank you!

@nolimits4web
Copy link
Owner

nolimits4web commented May 6, 2020

Thanks! But i can't accept this PR for few reasons:

  • we don't need web pack here, it doubles the output dist files with a lot of "garbage"
  • we still need esm modules to be there, why they removed?

Can you change it to modifying only src files? If not, i can merge it manually this weekend then

@rgolea
Copy link
Contributor Author

rgolea commented May 6, 2020

@nolimits4web To answer your questions and issues in order:

  • I know there is no need for webpack but I couldn't make gulp work under any circumstances. Also, I fear some ES6 and backward compatibility issues (that's why i added typescript)
  • I don't really know how to change to target esm, maybe it's something regarding the ts-loader options and adding another entrypoint? I'm not really sure on the difference as well.
  • I'm pretty sure we can add .d.ts files to the project and it will allow the interface to display all usages of future implemented methods.

I can definitely change the PR to only change the few parts but I cannot even test nor build the project and I'm afraid I might break something. Ping me back as soon as you can.

Thanks

@nolimits4web
Copy link
Owner

Ok, can you then just keep changes in src files, e.g. monkey patching itself + keep in TypeScript, and i will update the build process and rest of the stuff

@rgolea
Copy link
Contributor Author

rgolea commented May 6, 2020

You're a life saver! I will do it right now.

@rgolea
Copy link
Contributor Author

rgolea commented May 6, 2020

@nolimits4web do you want me to leave the tsconfig base file?

@nolimits4web
Copy link
Owner

Yes, keep it, I’ll tweak it if needed

@rgolea
Copy link
Contributor Author

rgolea commented May 6, 2020

@nolimits4web right now it builds in the build folder. You can also extends the base tsconfig file and have running multiple configs (like the output directory, modules usage like commonjs, etc). If anything, ping me and I will keep looking into it.

@nolimits4web nolimits4web merged commit 096e9e2 into nolimits4web:master May 8, 2020
@nolimits4web
Copy link
Owner

@rgolea i just released sir-window 2.x beta, can you try if it works for you:

npm i ssr-window@beta

@rgolea
Copy link
Contributor Author

rgolea commented May 8, 2020

@nolimits4web I'll try it this weekend. Thank you sooo much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants