Skip to content

[FIX] Gofor server lib require#32

Open
odeadglass wants to merge 2 commits intomasterfrom
gofor-server-lib-require
Open

[FIX] Gofor server lib require#32
odeadglass wants to merge 2 commits intomasterfrom
gofor-server-lib-require

Conversation

@odeadglass
Copy link
Copy Markdown

Content:

  • Migrate node-fetch require to work explicit for lib

Why:

  • Currently, when npm package uses webpack browser entry config and trying to require node-fetch, it will arrive as Module, There for there is a need to require it explicit from the lib to ensure a consistent behaviour.

Copy link
Copy Markdown
Contributor

@sahariko sahariko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I fully understand why this change is needed.
gofor/server should not be used by browser code.

@odeadglass
Copy link
Copy Markdown
Author

Well, We have a situation where a UI package provides a SDK aswel, currently I have no option to separate into different bundles, there for my SDK code getting bundled with the UI by webpack.

@omrilotan
Copy link
Copy Markdown
Member

omrilotan commented Dec 18, 2018

@odeadglass This is an open source solution, if you have internal limitations, please make sure to specify for the benefit of all consumers/contributors.

I think Oded's issue is that, in some bundled solutions a browser environment is assumed, and node-fetch will skip polyfilling.

While we've found a solution in the consumer side (define node environment in the bundled solution), it is still correct that gofor/server is explicitly a server entry.

Perhaps there's room for an identical gofor/isomorphic solution that relies on node-fetch's behaviour, Or modify the root entry to use node-fetch.

@sahariko WDYT?

@sahariko
Copy link
Copy Markdown
Contributor

Adding another isomorphic entry sounds like a good solution, @odeadglass do you mind refactoring your PR to implement this?

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.

3 participants