builder icon indicating copy to clipboard operation
builder copied to clipboard

Add support for Shopify's Hydrogen/Oxygen

Open Dkendal opened this issue 3 years ago • 4 comments

Description

Adds support for Shopify's Hydrogen and Oxygen project.

Changes:

Core

  • replace deprecated usage of require('url').parse with a solution based on new URL(...), verified that it behaves the same (for the most part) as the old function). Does have slightly different behavior for urls that don't have "slashes", for example: "http:example.com" will be parsed as "http://example.com", rather than being left as is.
  • fold the definition of requestUrl to just use fetch. Remove custom fetch polyfill that used http and https.
  • Type fixes for catch blocks where the value of error was assumed to be any. Values are now coerced to an Error.
  • Type error fix for fetch({mode: ...})
  • Fix issue with fetch polyfill. Changed to check if fetch is defined on globalThis to better suppport all environments rather than using global, which is only available in NodeJS.
  • Changed to be a composite TS project.

React

  • Fix issue with fetch polyfill. Changed to check if fetch is defined on globalThis to better suppport all environments rather than using global, which is only available in NodeJS.
  • Type fixes for catch blocks where the value of error was assumed to be any. Values are now coerced to an Error.
  • Fix issue with safeDynamicRequire which caused it to not function in the CloudFlare Worker environment. No longer assumes that eval is available but rather checks if require is a function, which should only be in NodeJS, and if that's the case it calls eval('require'). I don't know if the eval trick is still required to work around webpack, or under what circumstances.

As an aside, in the future we should probably replace all usages of safeDynamicRequire with just just dynamic import(...), which should be supported everywhere now - it's at least supported in all LTS versions of NodeJS.

Dkendal avatar Aug 06 '22 00:08 Dkendal

Glad to see you got this working! Now that I see it in actual code though, I am a little worried about the added complexity of having a rollup plugin versus changing the SDK code directly to work in all environments. We know that adding extra steps or complications acts as a roadblock for people to integrate, so ideally it would be nice if we could avoid requiring them to install a plugin to get builder to work in the oxygen environment. Plus the plugin would require us to maintain it as well as add additional docs for people to deal with.

On that note, forgive me if you already explained it to me (last week was a bit hectic sweat_smile ), but why do we need to use the rollup approach versus just modifying the functions directly to deal with the use of eval? For example if we changed safeDynamicRequire b690eab/packages/react/src/functions/safe-dynamic-require.ts to check for eval first (or also add a try catch if we need to), would that work and allow us to avoid having to maintain a separate plugin?

export const safeDynamicRequire: typeof require = Builder.isServer && typeof eval === 'function'
  ? eval('require')
  : ((() => null) as any);

or add in try catch something like

const getSafeRequire = () => {
  let safeDynamicRequire: typeof require; 
  if (Builder.isServer && typeof eval === 'function') {
    try { 
      safeDynamicRequire = eval('require') 
    } catch (e) { }
  }
  return safeDynamicRequire || ((() => null) as any);
}

And then wherever we use this fancy require for vm2, we could just return null whenever the require is null, since null is a valid value for a binding.

~Unfortunately in code checks for environment specific quirks like typeof eval ... won't work on miniflare. When the miniflare app boots it checks for invaid or unsupported features during the parse phase, and fails to boot if they are present.~

~This is a minimal reproduction: https://github.com/Dkendal/miniflare-demo-nodejs/blob/main/index.js~

Dkendal avatar Aug 08 '22 17:08 Dkendal

Looks like I was mistaken that cloudflare scans for code generation usages in the parse phase, it happens during runtime. Checking for the cf env is not great as you look for cache.default being defined but I can probably just change the sdk to have conditional branches for this like we do for react native

Dkendal avatar Aug 08 '22 17:08 Dkendal

@mrkoreye this is ready for review again

Dkendal avatar Aug 09 '22 19:08 Dkendal

description updated.

Dkendal avatar Aug 09 '22 20:08 Dkendal