Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

[quilt] Build Quilt using sewing-kit-next #1538

Closed
wants to merge 41 commits into from
Closed

[quilt] Build Quilt using sewing-kit-next #1538

wants to merge 41 commits into from

Conversation

b-ryu
Copy link
Contributor

@b-ryu b-ryu commented Jun 24, 2020

Description

Related to #1522

This PR integrates sewing-kit-next into Quilt's builds.

  • yarn build
  • yarn test
  • yarn lint

Some notes

  • I've noticed some intermittent issues with Watchman when trying to run tests, especially right after a fresh build. Usually running watchman shutdown-server and dev up after restarting the terminal after a while fixes things. I suspect it may have something to do with the volume of built files (note: look into configuring Watchman)
  • This PR bumps the maximum memory allowed to VS Code's builtin TS plugin. Adding a tsconfig.json to the workspace root causes the plugin to traverse the entire workspace when you open a .ts file not included in one of the referenced projects/packages (opening a .ts file in a project/package loads the TS for that package only, as before). This behaviour also occurs in other similarly structured workspaces, the difference being that Shopify/quilt is relatively large so the plugin appears to run out of memory halfway, causing it to hang and crash repeatedly. Increasing the memory limit seems to fix the problem, though it might be something to look into further

@b-ryu b-ryu force-pushed the 1522-sk-next branch 3 times, most recently from d3b5765 to e25b8e7 Compare June 30, 2020 21:02
@b-ryu b-ryu force-pushed the 1522-sk-next branch 2 times, most recently from 8df6187 to 29c9333 Compare July 9, 2020 16:51
@b-ryu b-ryu force-pushed the 1522-sk-next branch 3 times, most recently from 5fbbc8f to 9957891 Compare July 15, 2020 00:34
@b-ryu b-ryu changed the title [WIP][quilt] Build Quilt using sewing-kit-next [quilt] Build Quilt using sewing-kit-next Jul 15, 2020
@b-ryu b-ryu marked this pull request as ready for review July 15, 2020 01:38
@shopify-admins shopify-admins requested a review from a team July 21, 2020 14:22
Copy link
Contributor

@cejaekl cejaekl left a comment

Choose a reason for hiding this comment

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

I've only looked at the i18n files (on "behalf" of intl-globalization) but see no cause for concern there. OK by me, for what that's worth. :)

@b-ryu b-ryu force-pushed the 1522-sk-next branch 2 times, most recently from c382968 to 3505c07 Compare July 22, 2020 17:52
.eslintrc.js Outdated
'plugin:@shopify/react',
'plugin:@shopify/jest',
'plugin:@shopify/prettier',
'plugin:@sewing-kit/typescript',
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do these configs live?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Found it, https://github.com/lemonmade/sewing-kit/tree/main/packages/eslint-plugin/src/config

Not related to quilt. It's interesting that there's a@sewing-kit/eslint-plugin vs @sewing-kit/plugin-eslint. One is a plugin for sewing-kit and one is an eslint plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, github is being wonky with this big PR. Comment thread didn't' refresh and the page is freezing.

Copy link
Member

Choose a reason for hiding this comment

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

You should keep the Shopify ESLint plugins, I only added a custom ESLint plugin in sewing-kit to get rid of a few rules I hate, and show how it could be authored with TypeScript (unlike our actual plugin, that's all in JS)

@@ -23,6 +23,7 @@
"build": true,
"**/*.js": true
},
"typescript.tsserver.maxTsServerMemory": 8000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share some re-producible steps for the issues you had with VSCode, please? I can try to take a deeper look.

Copy link
Contributor Author

@b-ryu b-ryu Jul 22, 2020

Choose a reason for hiding this comment

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

Sure thing:

  1. Pull this branch down and open it in VS Code
  2. Get rid of the typescript.tsserver.maxTsServerMemory setting fix
  3. Close all tabs in and close VS Code
  4. Open Quilt in VS Code
  5. Open sewing-kit.config.ts in the workspace root
  6. On my editor at least, the builtin TS/JS plugin hangs and never stops initializing, as noted in the bottom toolbar. The plugin eventually restarts itself a couple times and then crashes, leaving a warning popup

Screen Shot 2020-07-22 at 2 52 57 PM

In VS Code you can use the command >Open TS Server log to see the plugin logs. For me, the logs just stop in the middle of loading a package around halfway through.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to repro this on first attempt. I tried a few hacks to see what would happen (eg. move the root tsconfig.json to packages/tsconfig.json). That seemed to help. I reverted my changes and cleaned out my git env and I'm not able to reproduce this anymore :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might have something to do with my own local setup then 🤔 @lemonmade @TheMallen is this an issue for you guys as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll try it out after our 1:1

return createComposedProjectPlugin<Package>('Quilt.Package', [
javascript(),
typescript(),
react(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We have some quilt packages that don't use any react (eg. address-consts). Do they still need the react plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, I can make that configurable via arguments instead 👍

createProjectTestPlugin,
createProjectBuildPlugin,
} from '@sewing-kit/plugins';
import {react} from '@sewing-kit/plugin-react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Something to keep in mind for the future. It would be nice to have some of these common plugins re-exported from @sewing-kit/plugins so devs can do something like:

import {
   // other stuff
  react, 
  javascript, 
  typescript,
 } from '@sewing-kit/plugins';

instead of

import {react} from '@sewing-kit/plugin-react';
import {javascript} from '@sewing-kit/plugin-javascript';
import {typescript} from '@sewing-kit/plugin-typescript';

@lemonmade do you foresee any issues with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the core plugins are annoying to remember what all the package names are

Copy link
Member

Choose a reason for hiding this comment

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

This is what composed plugins are for (e.g., https://github.com/lemonmade/quilt/blob/main/packages/sewing-kit-plugins/src/index.ts#L29), so I don't think it's needed.

};
}

export function quiltPackage({binaryOnly = true, jestEnv = 'jsdom'} = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this binaryOnly flag for? It's defaulted to true and I don't see it being used anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I just copied that from Chris' Quilt config. Wasn't sure if we wanted .mjs and .esnext files in our builds. I can get rid of it and hardcode that to be true instead for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave them out for now. Adding support for .mjs and .esnext feels out of scope in validating sewing-kit-next in quilt.

Copy link
Contributor

@marutypes marutypes Jul 22, 2020

Choose a reason for hiding this comment

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

disagree, one of the major benefits of using sewing-kit-next is that it outputs files that are actually tree-shakeable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I stand corrected. @b-ryu, sorry for the confusion. We'll need this.

@@ -24,7 +24,7 @@
},
"homepage": "https://github.com/Shopify/quilt/blob/master/packages/address-consts/README.md",
"files": [
"dist/*",
"build/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why these are all changing to build? Anyway we can keep it as dist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking through the plugin code I don't believe it's a hard requirement, but I think it's the preferred default. I'll see if keeping it as dist changes anything 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Something to verify. Do to generated root level entry points respect the output folder in the tsconfig.

  • Update the output folder in packages/web-worker/tsconfig.json to dist
  • Run a build
  • Verify that all the root level entry points generated for the following include a ./dist
  pkg.entry({root: './src/index'});
  pkg.entry({name: 'babel', root: './src/babel-plugin'});
  pkg.entry({name: 'webpack', root: './src/webpack-parts'});
  pkg.entry({name: 'worker', root: './src/worker'});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some digging. Apparently setting outDir in tsconfig.json only affects TS output. It seems that other outputs are hardcoded to go into the build folder (the generated entrypoints point to the proper respective folder).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for digging into this. I'm ok with standardizing on our tooling to use build as the default output dir. We'll just need to make sure we update all our cases where we hard-coded dist. @shopify/polyfills comes to mind. It's hurt us in the past.

We'll probably want an escape hatch out of this in the future though, unless it's already possible today (cc/ @lemonmade)

@@ -3,8 +3,8 @@
"version": "2.0.1",
"license": "MIT",
"description": "Constants and types relating to `@shopify/address`",
"main": "dist/src/index.js",
"types": "dist/src/index.d.ts",
"main": "index.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is outputting an index.js in the root a defaults from sewing-kit next? I personally like when all the build output is in one folder that way it's easier to undo/clear caches. Probably worth a discussion with the team though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can look into this; I think it's the default, not sure if it's configureable (based on PackageEntryOptions in SK-next). FWIW, I added a yarn clean script to package.json that clears away any built files/entrypoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

After seeing the use case for multiple entries into a package (eg. web-workers), this sounds like a reasonable tradeoff.

   pkg.entry({root: './src/index'});
   pkg.entry({name: 'babel', root: './src/babel-plugin'});
   pkg.entry({name: 'webpack', root: './src/webpack-parts'});
   pkg.entry({name: 'worker', root: './src/worker'});

This whole automation of a package entry is really neat!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't love littering the root of a package with all its entry files, but unfortunately there's not really any other way to do multi-entry packages consistently.

@@ -0,0 +1,76 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Does sewing-kit-next expect the tsconfig.json to live in the root dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not certain, but I believe so? I remember getting some errors about not finding a quilt/tsconfig.json when playing around with with different tsconfig.json setups.

package.json Outdated
"build": "sk build",
"check": "lerna run check",
"ci:lint-docs": "yarn generate docs && test -z \"$(git status --porcelain)\" || echo 'The root README has not been updated. Run `yarn generate docs` in the root of your quilt directory and try again.'",
"clean": "rimraf './packages/*/build' './packages/*/*.d.ts' './packages/*/*.js' './packages/*/*.esnext' './packages/*/*.mjs' '.sewing-kit'",
Copy link
Contributor

Choose a reason for hiding this comment

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

Clearing all these files manually feels like a guessing game to me. We should rely on sewing-kit to handle this.

There are a number of files that match the following './packages/*/*.d.ts' './packages/*/*.js' that are removed in this PR. Did any of these get accidentally picked up by a yarn run clean or are all of them covered by sewing-kit's pkg.entry automation?

I don't expect an answer for this 😅 Just more things to consider during our validation of sewing-kit-next.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I deliberately got rid of some packages/*/*.d.ts and packages/*/*.js files (also added them to .gitignore) since we didn't need them anymore with our new generated entrypoints, but it's probably worth it for me to go back and double-check that they were all actually just entrypoints 👍

Copy link
Contributor

@ismail-syed ismail-syed left a comment

Choose a reason for hiding this comment

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

Haven't looked into the VSCode issues yet. This is looking awesome so far! Love the automated entry points with pkg.entry. I'm afraid we're hitting a performance tripwire.

Before (c3baa5f) After (3505c07)
Cold 402.35s 634.54s
Warm 1.56s 148.41s

.eslintrc.js Outdated
'plugin:@shopify/react',
'plugin:@shopify/jest',
'plugin:@shopify/prettier',
'plugin:@sewing-kit/typescript',
Copy link
Member

Choose a reason for hiding this comment

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

You should keep the Shopify ESLint plugins, I only added a custom ESLint plugin in sewing-kit to get rid of a few rules I hate, and show how it could be authored with TypeScript (unlike our actual plugin, that's all in JS)

packages/*/*.node
packages/*/*.mjs
packages/*/*.d.ts
packages/*/*.js
Copy link
Member

Choose a reason for hiding this comment

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

The thing to watch out for here is that we sometimes nest .eslintrc.js or similar in subdirectories, and we do generally want to include those files.

.travis.yml Outdated
- yarn build --verbose
- yarn test:ci --testPathPattern react-server address
- yarn build
- yarn type-check
Copy link
Member

Choose a reason for hiding this comment

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

build effectively includes a type-check, because part of the build is generating the TypeScript outputs

hooks.configure.hook(hooks => {
hooks.jestEnvironment?.hook(() => jestEnv);

hooks.jestConfig?.hook(config => ({
Copy link
Member

Choose a reason for hiding this comment

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

There are more specific hooks for jestTransforms and the watch ignore patterns

package.json Outdated
"release": "yarn lint:changelogs && lerna publish",
"release-beta": "lerna publish --dist-tag beta",
"pretest": "yarn build",
"test": "yarn _test",
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be changing the default test command to be in watch mode, which may not be desirable

@@ -3,8 +3,8 @@
"version": "2.0.1",
"license": "MIT",
"description": "Constants and types relating to `@shopify/address`",
"main": "dist/src/index.js",
"types": "dist/src/index.d.ts",
"main": "index.js",
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't love littering the root of a package with all its entry files, but unfortunately there's not really any other way to do multi-entry packages consistently.

"main": "dist/src/index.js",
"types": "dist/src/index.d.ts",
"main": "index.js",
"types": "index.d.ts",
Copy link
Member

Choose a reason for hiding this comment

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

For packages to get the full benefits of the different build outputs, you'll eventually need to also add esnext/ module entries to package.json (and, if you're interested, the future-looking exports map). Example: https://github.com/lemonmade/quilt/blob/main/packages/quilt/package.json#L10-L32

Brian Ryu added 17 commits August 10, 2020 16:14
Get rid of extra TS version
 - @shopify/address-consts@1.0.0-sknext-beta.1
 - @shopify/address-mocks@1.0.0-sknext-beta.1
 - @shopify/address@1.0.0-sknext-beta.1
 - @shopify/admin-graphql-api-utilities@1.0.0-sknext-beta.1
 - @shopify/ast-utilities@1.0.0-sknext-beta.1
 - @shopify/async@1.0.0-sknext-beta.1
 - @shopify/browser@1.0.0-sknext-beta.1
 - @shopify/csrf-token-fetcher@1.0.0-sknext-beta.1
 - @shopify/css-utilities@1.0.0-sknext-beta.1
 - @shopify/dates@1.0.0-sknext-beta.1
 - @shopify/decorators@1.0.0-sknext-beta.1
 - @shopify/enzyme-utilities@1.0.0-sknext-beta.1
 - @shopify/function-enhancers@1.0.0-sknext-beta.1
 - @shopify/graphql-persisted@1.0.0-sknext-beta.1
 - @shopify/graphql-testing@1.0.0-sknext-beta.1
 - @shopify/i18n@1.0.0-sknext-beta.1
 - @shopify/jest-dom-mocks@1.0.0-sknext-beta.1
 - @shopify/jest-koa-mocks@1.0.0-sknext-beta.1
 - @shopify/jest-mock-apollo@1.0.0-sknext-beta.1
 - @shopify/jest-mock-router@1.0.0-sknext-beta.1
 - @shopify/koa-liveness-ping@1.0.0-sknext-beta.1
 - @shopify/koa-metrics@1.0.0-sknext-beta.1
 - @shopify/koa-performance@1.0.0-sknext-beta.1
 - @shopify/koa-shopify-auth@1.0.0-sknext-beta.1
 - @shopify/koa-shopify-graphql-proxy@1.0.0-sknext-beta.1
 - @shopify/koa-shopify-webhooks@1.0.0-sknext-beta.1
 - @shopify/logger@1.0.0-sknext-beta.1
 - @shopify/magic-entries-webpack-plugin@1.0.0-sknext-beta.1
 - @shopify/mime-types@1.0.0-sknext-beta.1
 - @shopify/network@1.0.0-sknext-beta.1
 - @shopify/performance@1.0.0-sknext-beta.1
 - @shopify/polyfills@1.0.0-sknext-beta.1
 - @shopify/predicates@1.0.0-sknext-beta.1
 - @shopify/react-app-bridge-universal-provider@1.0.0-sknext-beta.1
 - @shopify/react-async@1.0.0-sknext-beta.1
 - @shopify/react-bugsnag@1.0.0-sknext-beta.1
 - @shopify/react-compose@1.0.0-sknext-beta.1
 - @shopify/react-cookie@1.0.0-sknext-beta.1
 - @shopify/react-csrf-universal-provider@1.0.0-sknext-beta.1
 - @shopify/react-csrf@1.0.0-sknext-beta.1
 - @shopify/react-effect@1.0.0-sknext-beta.1
 - @shopify/react-form-state@1.0.0-sknext-beta.1
 - @shopify/react-form@1.0.0-sknext-beta.1
 - @shopify/react-google-analytics@1.0.0-sknext-beta.1
 - @shopify/react-graphql-universal-provider@1.0.0-sknext-beta.1
 - @shopify/react-graphql@1.0.0-sknext-beta.1
 - @shopify/react-hooks@1.0.0-sknext-beta.1
 - @shopify/react-html@1.0.0-sknext-beta.1
 - @shopify/react-hydrate@1.0.0-sknext-beta.1
 - @shopify/react-i18n-universal-provider@1.0.0-sknext-beta.1
 - @shopify/react-i18n@1.0.0-sknext-beta.1
 - @shopify/react-idle@1.0.0-sknext-beta.1
 - @shopify/react-import-remote@1.0.0-sknext-beta.1
 - @shopify/react-intersection-observer@1.0.0-sknext-beta.1
 - @shopify/react-network@1.0.0-sknext-beta.1
 - @shopify/react-performance@1.0.0-sknext-beta.1
 - @shopify/react-router@1.0.0-sknext-beta.1
 - @shopify/react-server@1.0.0-sknext-beta.1
 - @shopify/react-shortcuts@1.0.0-sknext-beta.1
 - @shopify/react-testing@1.0.0-sknext-beta.1
 - @shopify/react-tracking-pixel@1.0.0-sknext-beta.1
 - @shopify/react-universal-provider@1.0.0-sknext-beta.1
 - @shopify/react-web-worker@1.0.0-sknext-beta.1
 - @shopify/rpc@1.0.0-sknext-beta.1
 - @shopify/semaphore@1.0.0-sknext-beta.1
 - @shopify/sewing-kit-koa@1.0.0-sknext-beta.1
 - @shopify/statsd@1.0.0-sknext-beta.1
 - @shopify/useful-types@1.0.0-sknext-beta.1
 - @shopify/web-worker@1.0.0-sknext-beta.1
 - @shopify/with-env@1.0.0-sknext-beta.1
We're no longer using the TS builds
 - @shopify/address-consts@1.0.0-sknext-beta.2
 - @shopify/address-mocks@1.0.0-sknext-beta.2
 - @shopify/address@1.0.0-sknext-beta.2
 - @shopify/admin-graphql-api-utilities@1.0.0-sknext-beta.2
 - @shopify/ast-utilities@1.0.0-sknext-beta.2
 - @shopify/async@1.0.0-sknext-beta.2
 - @shopify/browser@1.0.0-sknext-beta.2
 - @shopify/csrf-token-fetcher@1.0.0-sknext-beta.2
 - @shopify/css-utilities@1.0.0-sknext-beta.2
 - @shopify/dates@1.0.0-sknext-beta.2
 - @shopify/decorators@1.0.0-sknext-beta.2
 - @shopify/enzyme-utilities@1.0.0-sknext-beta.2
 - @shopify/function-enhancers@1.0.0-sknext-beta.2
 - @shopify/graphql-persisted@1.0.0-sknext-beta.2
 - @shopify/graphql-testing@1.0.0-sknext-beta.2
 - @shopify/i18n@1.0.0-sknext-beta.2
 - @shopify/jest-dom-mocks@1.0.0-sknext-beta.2
 - @shopify/jest-koa-mocks@1.0.0-sknext-beta.2
 - @shopify/jest-mock-apollo@1.0.0-sknext-beta.2
 - @shopify/jest-mock-router@1.0.0-sknext-beta.2
 - @shopify/koa-liveness-ping@1.0.0-sknext-beta.2
 - @shopify/koa-metrics@1.0.0-sknext-beta.2
 - @shopify/koa-performance@1.0.0-sknext-beta.2
 - @shopify/koa-shopify-auth@1.0.0-sknext-beta.2
 - @shopify/koa-shopify-graphql-proxy@1.0.0-sknext-beta.2
 - @shopify/koa-shopify-webhooks@1.0.0-sknext-beta.2
 - @shopify/logger@1.0.0-sknext-beta.2
 - @shopify/magic-entries-webpack-plugin@1.0.0-sknext-beta.2
 - @shopify/mime-types@1.0.0-sknext-beta.2
 - @shopify/network@1.0.0-sknext-beta.2
 - @shopify/performance@1.0.0-sknext-beta.2
 - @shopify/polyfills@1.0.0-sknext-beta.2
 - @shopify/predicates@1.0.0-sknext-beta.2
 - @shopify/react-app-bridge-universal-provider@1.0.0-sknext-beta.2
 - @shopify/react-async@1.0.0-sknext-beta.2
 - @shopify/react-bugsnag@1.0.0-sknext-beta.2
 - @shopify/react-compose@1.0.0-sknext-beta.2
 - @shopify/react-cookie@1.0.0-sknext-beta.2
 - @shopify/react-csrf-universal-provider@1.0.0-sknext-beta.2
 - @shopify/react-csrf@1.0.0-sknext-beta.2
 - @shopify/react-effect@1.0.0-sknext-beta.2
 - @shopify/react-form-state@1.0.0-sknext-beta.2
 - @shopify/react-form@1.0.0-sknext-beta.2
 - @shopify/react-google-analytics@1.0.0-sknext-beta.2
 - @shopify/react-graphql-universal-provider@1.0.0-sknext-beta.2
 - @shopify/react-graphql@1.0.0-sknext-beta.2
 - @shopify/react-hooks@1.0.0-sknext-beta.2
 - @shopify/react-html@1.0.0-sknext-beta.2
 - @shopify/react-hydrate@1.0.0-sknext-beta.2
 - @shopify/react-i18n-universal-provider@1.0.0-sknext-beta.2
 - @shopify/react-i18n@1.0.0-sknext-beta.2
 - @shopify/react-idle@1.0.0-sknext-beta.2
 - @shopify/react-import-remote@1.0.0-sknext-beta.2
 - @shopify/react-intersection-observer@1.0.0-sknext-beta.2
 - @shopify/react-network@1.0.0-sknext-beta.2
 - @shopify/react-performance@1.0.0-sknext-beta.2
 - @shopify/react-router@1.0.0-sknext-beta.2
 - @shopify/react-server@1.0.0-sknext-beta.2
 - @shopify/react-shortcuts@1.0.0-sknext-beta.2
 - @shopify/react-testing@1.0.0-sknext-beta.2
 - @shopify/react-tracking-pixel@1.0.0-sknext-beta.2
 - @shopify/react-universal-provider@1.0.0-sknext-beta.2
 - @shopify/react-web-worker@1.0.0-sknext-beta.2
 - @shopify/rpc@1.0.0-sknext-beta.2
 - @shopify/semaphore@1.0.0-sknext-beta.2
 - @shopify/sewing-kit-koa@1.0.0-sknext-beta.2
 - @shopify/statsd@1.0.0-sknext-beta.2
 - @shopify/useful-types@1.0.0-sknext-beta.2
 - @shopify/web-worker@1.0.0-sknext-beta.2
 - @shopify/with-env@1.0.0-sknext-beta.2
@@ -32,7 +32,7 @@ export const polyfills: {[polyfill: string]: PolyfillDescriptor} = {
export function mappedPolyfillsForEnv(
env: 'node' | 'jest' | string[],
): {[key: string]: string} {
const prefix = `@shopify/polyfills/build/cjs`;
const prefix = `@shopify/polyfills`;
Copy link
Contributor Author

@b-ryu b-ryu Aug 11, 2020

Choose a reason for hiding this comment

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

I'm wondering if it would be a terrible thing to just have the CommonJS builds for @shopify/polyfills and just have this map to build/cjs instead. Not sure what the impact on treeshaking would be, since this package has sideEffects set to true anyway.

@b-ryu
Copy link
Contributor Author

b-ryu commented Aug 13, 2020

Since the SK-next project is still in the exploration phase, we've decided to hold off on merging/releasing this until the build phase. Closing this PR for now.

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

Successfully merging this pull request may close these issues.

5 participants