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

Full "webDependencies" management #236

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Conversation

FredKSchott
Copy link
Owner

@FredKSchott FredKSchott commented Mar 4, 2020

-  "snowpack": {
-    "webDependencies": [
-      "a",
-      "b",
-      "c"
-    ]
-  },
-  "dependencies": {
+  "webDependencies": {
    "a": "^1.2.3"
    "b": "^1.2.3"
    "c": "^1.2.3"
  },

Background

Keeping up the momentum from #228: Snowpack still relies on semver info to be stored in your package.json dependencies. This messes with our "install from the Pika CDN" story, since that means that npm will still try to install all of your dependencies. For now, all we've really done is added a second, potentially confusing install step.

If Snowpack is going to manage your install step for you, that means we need to own the semver data in a place that npm won't try to install from. We've always used the "webDependencies" as a list of local dependencies to install, but in this PR I'd like us to add support for a new, top-level, "dependencies-like" semver object.

Implementation

  • Add support to read "webDependencies" from the top-level package.json (still supports reading it from snowpack config file / "snowpack" package.json config)
  • Add support for a "webDependencies" object notation, which holds semver data. No breaking changes, so we should still support an array of dependency names for now.
  • If "webDependencies" is object notation, default to --source pika.
  • Think through a new config val so that we can still pass additional, non-package assets/entrypoints (assets: []? include: []?) A: entrypoints for all additional entrypoints.
  • Simplify internal config datastructure to normalize this new support (we don't want rest of the package to have to worry about whether "webDependencies" is object vs. array).
  • Add tests, and in general do a cleanup pass.

Creating this PR now for early feedback, specifically on the public interface changes.
/cc @DangoDev @alex-saunders

src/config.ts Outdated
@@ -17,7 +17,7 @@ type DeepPartial<T> = {
// interface this library uses internally
export interface SnowpackConfig {
source: 'local' | 'pika';
webDependencies?: string[];
webDependencies?: string[] | {[packageName: string]: string};
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯! TypeScript is so nice. It’s hard to remember what supporting different data structures for backwards compatibility was like before typed JS

@drwpow
Copy link
Collaborator

drwpow commented Mar 5, 2020

This makes sense to me. +1 for the object format you went with. In general maps are great, easy structures to work with. And mirroring package.json’s dependencies structure is a nice bonus. Reasoning makes sense to me too.

Think through a new config val so that we can still pass additional, non-package assets/entrypoints (assets: []? include: []?)

What is the support story here? Is this an improvement to the --include flag or is this for a different case?

@FredKSchott
Copy link
Owner Author

Yea, this is something new (if webDependencies becomes an object, we still need a way to define multiple entrypoints inside of one package). You're right that we already use include for something else (src scanning), so we'd need to rename that first if we used that.

"entrypoints" or "additionalEntrypoints" could be good names as well.

@drwpow
Copy link
Collaborator

drwpow commented Mar 5, 2020

Got it. Yeah I’d vote for entryPoints: [] or packageEntryPoints: [] or something along those lines. That seems to be what Node calls these.

@ChristopherBiscardi
Copy link
Contributor

rolling #237 conversation into here, we could either continue to support a sub-extension on the types (extending string into [string, string], and similar for objects), or we could support a separate aliases key. I found that by declaring an alias, it eliminates the ability to use the automatic install methods because the aliases as described in #201 live in webDependencies. So if you want an alias in the current verison of #237, you have to opt-into manual webDependency management.

For these reasons, it might be nice if aliases was separate instead of merged into the webDependency structure. It could use the same kind of format as today, but instead of an array maybe an object?

"webDependencies": {
   "a": "^1.2.3"
   "b": "^1.2.3"
   "c": "^1.2.3"
 },
"aliases": {
  "react": "c"
 }

We basically need ways to add the alias to the importMap:

importMap.imports[alias] = `./${targetName}.js${hashQs}`;

add the aliases to rollupPluginAlias

     rollupPluginAlias({
        entries: Array.from(allInstallSpecifiers)
          .filter(spec => Array.isArray(spec))
          .map(([alias, mod]) => ({find: alias, replacement: mod})),
      }),

then the resolveRemote also doesn't have to change in #237 because if you specify aliases they have to be in package.json or webDependencies.

@ChristopherBiscardi ChristopherBiscardi mentioned this pull request Mar 6, 2020
3 tasks
@alex-saunders
Copy link
Collaborator

Just throwing my two cents into the mix here:

  • Love the move to a top-level entry for webDependencies, this feels much more natural and defines a clear split between node and web dependencies without a confusing middle-step.
  • The object notation is great and feels much more in-keeping with what users are familiar with in terms of dependency definition/management, I totally understand not wanting to introduce a breaking change but I think it would be great to phase-out the array syntax in future (2 syntax choices for the same information could definitely cause confusion)
  • Splitting assets from the webDependencies entry and keeping only package dependency definitions together sounds smart and should help to keep things clear and self-contained!
    • On this, I think either assets or webAssets would be a good name (as mentioned previously, include is too similar to different functionality already present within snowpack)
    • As mentioned by @ChristopherBiscardi , I think a separate aliases (webAliases?) key makes sense with this new structure. Keeping different data definitions in their own package json entries seems to be the way to go here and should help to reduce confusing datastructure logic within snowpack.

Loving the direction snowpack is headed in! It feels like it won't be long before we're a fully-fledged package manager 👀

@FredKSchott FredKSchott force-pushed the wip-webdependencies branch 2 times, most recently from 5f437b0 to c616389 Compare April 3, 2020 00:05
@FredKSchott FredKSchott changed the title [WIP] Full "webDependencies" management Full "webDependencies" management Apr 3, 2020
@FredKSchott FredKSchott marked this pull request as ready for review April 3, 2020 00:06
@FredKSchott FredKSchott force-pushed the wip-webdependencies branch 4 times, most recently from dccb87b to 6232a5a Compare April 3, 2020 19:16
@FredKSchott
Copy link
Owner Author

Thanks for the feedback everyone! Finally picking this back up after leaving it hanging for way too long. PR is now ready for review, with new tests and personally tested on pika.dev.

Here's an overview of how the pika.dev package.json changed for the new mode:

# Examples package.json
   "snowpack": {
-    "webDependencies": [
-      "@apollo/react-hooks",
-      "@emotion/core",
-      "@polymer/ristretto",
-      "@sentry/browser",
-      "@mountaingapsolutions/include",
-      "apollo-cache-inmemory",
-      "hardtack",
-      "apollo-boost",
-      "history",
-      "react",
-      "react-dom",
-      "tslib",
-      "react-router-dom",
+    "entrypoints": [
       "bulma/css/bulma.min.css",
+      "emoji-mart",
       "emoji-mart/css/emoji-mart.css",
       "github-markdown-css/github-markdown.css",
       "highlight.js/styles/default.css"
     ]
   },
-  "devDependencies": {
+  "webDependencies": {
     "@apollo/react-hooks": "^3.0.1",
     "@emotion/core": "^10.0.16",
     "@flourish/semver": "^1.0.2",
     "@github/textarea-autosize": "^0.1.0",
     "@mountaingapsolutions/include": "^1.0.0",
     "@polymer/ristretto": "^0.3.1",
     "@sentry/browser": "5.7.1",
-    "@types/react": "^16.9.2",
-    "@types/react-dom": "^16.9.0",
-    "@types/react-router-dom": "^5.1.0",
-    "algoliasearch": "^3.35.0",
     "apollo-boost": "^0.4.4",
-    "bulma": "^0.7.5",
-    "emoji-mart": "^2.11.1",
     "epoch-timeago": "^1.1.9",
     "formik": "^1.5.8",
-    "github-markdown-css": "^3.0.1",
     "hardtack": "^4.1.0",
-    "highlight.js": "^9.15.10",
     "history": "^4.9.0",
     "marked": "^0.7.0",
     "qss": "^2.0.3",
-    "react": "npm:@reactesm/react",
-    "react-dom": "npm:@reactesm/react-dom",
+    "react": "^16.13.0",
+    "react-dom": "^16.13.0",
     "react-instantsearch-core": "^5.7.0",
     "react-instantsearch-dom": "^5.7.0",
     "react-meta-elements": "^1.0.0",
@@ -68,5 +45,16 @@
     "react-switch": "^5.0.1",
     "tslib": "^1",
     "yup": "^0.27.0"
+  },
+  "devDependencies": {
+    "@types/react": "^16.9.2",
+    "@types/react-dom": "^16.9.0",
+    "@types/react-router-dom": "^5.1.0",
+    "algoliasearch": "^3.35.0",
+    "bulma": "^0.7.5",
+    "emoji-mart": "^2.11.1",
+    "github-markdown-css": "^3.0.1",
+    "highlight.js": "^9.15.10"
   }

Some good learnings from this implementation, to spin off into future discussions:

  • Still so happy that we've gotten rid of the React workaround packages!
  • @types/* still need to exist locally, so that they exist in your node_modules for TypeScript. However, we could automate this (CDN ships type definitions automatically, we'd just install them into node_modules/@types/xyz ourselves).
  • Our CDN can handle raw assets like CSS. Is there a way to put "github-markdown-css/github-markdown.css": "^3.0.1" into your webdependencies so that you don't need an additional entrypoints entry?
  • Our CDN can handle basic exports like preact/hooks. But right now you'll still need to put "preact" into your devDependencies and then "preact/hooks" into your entrypoints. We should support "preact/hooks" in the entrypoints array without needing it in "devDependencies".
  • entrypoints or additionalEntrypoints? Now that every package in your webDependencies is installed automatically, really additionalEntrypoints is most fitting.

@FredKSchott FredKSchott requested a review from drwpow April 3, 2020 19:17
@FredKSchott FredKSchott force-pushed the wip-webdependencies branch 2 times, most recently from 3595638 to 3f2d1bb Compare April 3, 2020 21:17
@FredKSchott FredKSchott merged commit 06f86bb into master Apr 7, 2020
@FredKSchott FredKSchott deleted the wip-webdependencies branch April 7, 2020 01:05
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

4 participants