Support custom directory for installed dependencies

FredKSchott commented 4 years ago

Original Discussion: https://www.pika.dev/npm/snowpack/discuss/308 /cc @jhfbosman, @FredKSchott

Right now, we hardcode that your dependencies live at "/web_modules/" in your deployed application. This was mainly out of convenience for our path rewriting logic, and not any real technical limitation.

A user should be able to customize where their web_modules directory is mounted to, and our dev/build workflows should support this. I believe this will only require a few fixes in our "import rewriting" step to support.

Note: Make sure that https://github.com/pikapkg/snowpack/issues/448 is tackled as a part of this

danprince commented 4 years ago

Hey @FredKSchott happy to take this one if you want. Does configuring this through a new top-level config option like webModulesDir sound ok to you?

Should this also respect the dest option from the install options? With /web_modules hardcoded into the path rewriting I guess that option isn't used much right now?

Snowpack is :+1: I'd be more than happy to contribute!

FredKSchott commented 4 years ago

Hey Dan, that sounds great! I can give a quick overview of what's needed:

scan-imports.ts :: webModulesIndex - This webModulesIndex check is actually legacy Snowpack v1.0, and is safe to remove entirely. No one is importing things from /web_modules directly in their source code anymore, instead letting the dev server / build resolve package names for them automatically.
scan-imports.ts :: scanImports() - This will need to be updated for the new config.
stats-formatter.ts - Honestly, I'd keep this. It's conceptually still the web_modules directory, we'll keep calling it your "web_modules" directory, so it makes sense to print it in the output.
build.ts & dev.ts - This is the meat of the actual change, where you'll actually need to make the change so that build & dev resolve dependency imports to the proper directory.

That should cover everything, but feel free to reach out with any questions.

FredKSchott commented 4 years ago

As for the config itself, I'd try to keep the mount:web_modules build script syntax if you can for now. We may eventually move to a more traditional config for mounting directories, but for now we actually already have built-in handling for this in config.ts and already even document it in the docs site (incorrectly, since it's not supported yet :)

"scripts": {
  "mount:web_modules": "mount $WEB_MODULES --to /static/web_modules"
}

hamstu commented 4 years ago

@FredKSchott Somewhat related to this I was trying to get create-snowpack-app (react) working inside Electron a few days ago. In Electron-land you usually load from a server during dev (so snowpack dev was great 👌 ) but in a packaged app you'll load static html (i.e., file://${__dirname}/index.html}).

The latter wasn't working since the entrypoint JS module was trying to import React from /web_modules, which resolved to my Mac's actual root dir 😅 .

Anyway, I solved it with some clever Electron magic, but I'm wondering if something related to this might be helpful there?

Two more things, 1) I love Snowpack! ❤️ and 2) would a create-snowpack-app template for Electron be helpful, or is that sort of out of scope? Would be happy it contribute if you like the idea. (Oh and sorry for semi-hijacking this issue! 🙈 )

FredKSchott commented 4 years ago

If you found Snowpack to be a good fit for Electron, I think that could be a great community template to add to the README! I think it would be out of scope for our team to maintain for now

It would also be great to add an "Electron" recipe/guide to our main docs site, with a link to https://gist.github.com/hamstu/b1f88ea486508111668df8d39e2d0ca1 for future users.

To your specific issue, I don't this solves it, since this is just customizing the final path (but it would still exist at / in either case.)

danprince commented 4 years ago

If I remove the webModulesIndex it would also make sense to go and change all the tests that import from /web_modules directly, right? For example:

https://github.com/pikapkg/snowpack/blob/5366dfbd6b0ee60f3e812aaa83480c27e53fc7dd/test/integration/include-ts/dir/a.ts#L5

If importing from /web_modules directly is no longer supported, does it make sense to keep the checks for an extension? E.g. if /web_modules/vue-router.js was a valid import then should vue-router.js be a valid import now?

How does that work with packages that have a .js in their name? E.g. pixi is a different package to pixi.js.

Is there an example of the best way to read args from the normalized scripts? I can see the mount:web_modules script in config.scripts but is looping through that array looking for the toUrl arg is the best way to get that value later on.

Final question (I promise!). What's the best way write an integration test that tests the import rewriting? I can't see any existing tests that do that, but I haven't dived too deep into runner.ts yet, so there might be something there already?

FredKSchott commented 4 years ago

All great questions!

Lets leave the scan-imports.ts :: webModulesIndex work for a separate PR later. Since this usage is already deprecated, leaving this hardcoded to /web_modules for now won't actually affect anyone and will continue to work in our tests, and this also removes the risk that we accidentally make a breaking change in this PR.
IIRC we handle the pixi vs. pixi.js somewhere else in the install step. But, like the previous point, this is only an issue for that part of the scanner that is now deprecated, so this shouldn't be something that you need to worry about.
Hmm, that's a good point. In the build.ts, there's an example of pulling the bundleWorker script out of the config.scripts array for later use. You could do something for the web_modules mount script: const webModulesScript = config.scripts.find((s) => s.id === 'mount:web_modules');
- I have a feeling we'll be revisiting this mount syntax soon, and maybe moving it to its own "mount": {...} top-level config. But until we make that change, it would be really great to keep it using the current script format.
Testing: https://github.com/pikapkg/snowpack/pull/456 adds our first build tests, once that's merged you could add a similar test. But don't let that block you.

danprince commented 4 years ago

Ok, so I've been getting stuck into the build command this evening. It's been slow going but I've got a new round of questions.

The default mount:web_modules script works fine because the fromDisk argument is hardcoded.

https://github.com/pikapkg/snowpack/blob/18a213b293b9b83c5cae7d16a96d7a8c314dae86/src/config.ts#L378-L391

Here's an example of the script I've been using to try and mount the $WEB_MODULES dir at another directory, to understand how it works.

"mount:test": "mount $WEB_MODULES --to /static/web_modules"

When the "mount" scripts are parsed there doesn't seem to be anything that evaluates the variables, so fromDisk ends up as the literal value "$WEB_MODULES/".

https://github.com/pikapkg/snowpack/blob/18a213b293b9b83c5cae7d16a96d7a8c314dae86/src/config.ts#L354-L359

When the build script eventually tries to run that mount script, nothing is copied to /static/web_modules.

I can't actually see where that $WEB_MODULES variable is supposed to be defined or any part of the code that handles mount scripts where any kind of variable interpolation happens.

FredKSchott commented 4 years ago

lol, I just realized that $WEB_MODULES was a placeholder, and never meant to do anything since the actual value was hardcoded. You can't find the handling of this because it doesn't exist!

You'll want to do something like this in config.ts:

// ...
if (scriptType === 'mount') {
  // ...
  if (scriptId === 'mount:web_modules') {
+    dirDisk =  process.env.NODE_ENV === 'production' ? BUILD_DEPENDENCIES_DIR : DEV_DEPENDENCIES_DIR;
  }
}
processedScripts.push(newScriptConfig);

danprince commented 4 years ago

Longer term I think I'd prefer to handle the web_modules location with an option that wasn't a mount script.

It doesn't really feel comparable to other mount scripts because it's an opaque directory that lives outside your project on disk (as opposed to your src, public etc).

That means that we're always going to be quietly ignoring whatever you pass as the first argument in the script.

"mount:web_modules": "mount i_will_be_ignored --to /web_modules_2"

What about controlling it through a buildOptions.webModulesDir (or similar) property instead? Or alternatively there's documented installOptions.dest:

FredKSchott commented 4 years ago

Yea, I'm more and more convinced that that's a good direction to go, maybe even for all mount configuration logic (we already went that direction with proxy, and i think everyones been pretty happy with it so far).

I'll spin up an issue to discuss a new design seperately. Thanks for tackling this!!!

ryands17 commented 4 years ago

@hamstu Could you share the electron example that you have created? I can't seem to be able to figure out the relative paths with web_modules.

CxRes commented 4 years ago

@FredKSchott I also would like to use snowpack with electron. However, I am not sure how to use in my setup which has both web and electron frontends which largely share identical code.

Currently, I "rollup" two bundles for the part of the code that depend on node built-ins, using polyfills for the web versions. I wonder if there is a way to:

Have snowpack bundle node polyfills separately, so that they are picked up at run time when running from the browser. or
create two separate bundles for each of the two cases.

FredKSchott commented 4 years ago

That's interesting. You could use installOptions.rollup.plugins to create a custom install plugin that basically marked all Node.js builtins as external. This would look something like:

// note: I'm writing this from memory, might be wrong
// see https://github.com/sindresorhus/is-builtin-module
[{resolveId(id) { return isNodeBuiltIn(id) ? {id, external: true} : null }]

CxRes commented 4 years ago

You should be able to do that by using '@rollup/plugin-node-resolve' using the 'preferBuiltins' option. But snowpack complains with, say, import path from 'path' (I suspect this is a bug. Should I open a separate ticket?):

⠼ snowpack installing... path
× "path" (Node.js built-in) could not be resolved. (https://www.snowpack.dev/#node-built-in-could-not-be-resolved). See https://www.snowpack.dev/#troubleshooting

Error: Entry module cannot be external (path).
    at error (D:\dev\Proto\snow-test\node_modules\.pnpm\rollup@2.18.1\node_modules\rollup\dist\shared\rollup.js:5154:30)
    at ModuleLoader.loadEntryModule (D:\dev\Proto\snow-test\node_modules\.pnpm\rollup@2.18.1\node_modules\rollup\dist\shared\rollup.js:17858:20)
    at async Promise.all (index 0)

But even with the above resolved i.e. if am to exclude node-builtins, I will only be able to use it with electron (and not in the browser). My dual use purpose is not achieved. I realize this is not an intended use case for snowpack, but it seems like a natural extension - in this case, I am trying to avoid running two builds (instead of the usual one), every time I make changes to my code.

FredKSchott commented 4 years ago

Yup, that makes sense. It out of scope to support both today. I'd actually love to see an example of something that uses Node builtins today (no polyfills) that runs in both the browser and electron, since I'm not sure how that would work. Unless you mean to only polyfill on the web and not polyfill in electron, but by definition that would ned to be two builds regardless.

izhar295 commented 2 years ago

Hi All,

I am trying to upgrade the snowpack version from 2.6.4 to 3.8.7. My current snowpack configuration for snowpack is given below: { "name": "web", "version": "3.2.29", "description": "CCUC Web micro-service", "private": true, "type": "module", "scripts": { "start": "cd server && node --es-module-specifier-resolution=node index.js", "bump": "bump", "watch": "cd server && nodemon --experimental-modules --es-module-specifier-resolution=node index.js", "test": "npm run test:ui && npm run test:server", "lint": "npm run eslint && npm run stylelint", "prepare": "snowpack install && sh ./scripts/terser.sh", "test:server": "cd server && jest --coverage --ci -c ../jest.server.config.json", "test:ui": "sh scripts/webtocommon.sh && jest -u --coverage --ci -c jest.ui.config.json && sh scripts/commontoweb.sh", "updatesnapshot:ui": "sh webtocommon.sh && jest --updateSnapshot --coverage --ci -c jest.ui.config.json && sh commontoweb.sh", "stylelint": "stylelint --fix --config .stylelintrc.json ui/asset/css", "eslint": "eslint -c .eslintrc.json .", "eslint:jenkins": "eslint -c .eslintrc.json . -f checkstyle > eslint.xml", "docker": "npm run prepare && docker build --compress -t web .", "eslint:server": "eslint -c .eslintrc.json ./server -f checkstyle > ./server/eslint.xml", "eslint:ui": "eslint -c .eslintrc.json ./ui -f checkstyle > ./ui/eslint.xml" }, "keywords": [], "author": "GG Team", "license": "UNLICENSED", "devDependencies": { "@babel/cli": "^7.7.5", "@babel/core": "^7.10.5", "@babel/plugin-proposal-class-properties": "^7.10.4", "@babel/plugin-syntax-dynamic-import": "^7.7.4", "@babel/plugin-transform-async-to-generator": "^7.10.4", "@babel/plugin-transform-runtime": "^7.10.5", "@babel/preset-env": "^7.10.4", "@babel/preset-react": "^7.10.4", "@testing-library/jest-dom": "^5.11.1", "babel-jest": "^26.1.0", "babel-plugin-minify-builtins": "^0.5.0", "babel-plugin-minify-dead-code-elimination": "^0.5.1", "babel-polyfill": "^6.0.16", "babel-preset-minify": "^0.5.1", "browser-resolve": "^1.11.3", "chai": "^4.2.0", "enzyme": "^3.11.0", "enzyme-adapter-react-16": "^1.15.2", "enzyme-to-json": "^3.5.0", "eslint": "^7.5.0", "eslint-config-standard": "^14.1.0", "eslint-plugin-import": "^2.22.0", "eslint-plugin-node": "^11.1.0", "eslint-plugin-promise": "^4.2.1", "eslint-plugin-react": "^7.20.3", "eslint-plugin-react-hooks": "^4.0.6", "eslint-plugin-standard": "^4.0.1", "jest": "^26.1.0", "jest-canvas-mock": "^2.2.0", "jest-enzyme": "^7.1.2", "jest-junit": "^11.0.1", "jsdom": "^16.3.0", "make-dir-cli": "^2.0.0", "nodemon": "^2.0.4", "openapi3-generator": "^0.13.0", "react-test-renderer": "^16.13.1", "snowpack": "^2.6.4", "stylelint": "^13.5.0", "stylelint-config-standard": "^20.0.0", "supertest": "^4.0.2", "terser": "^4.8.0" }, "dependencies": { "aws-sdk": "^2.683.0", "babel-eslint": "^10.1.0", "chart.js": "^2.9.4", "chartjs-plugin-labels": "^1.1.0", "express": "^4.17.1", "htm": "^3.0.4", "kafkajs": "^1.15.0", "mathjs": "^7.2.0", "mysql2": "^2.3.2", "node-fetch": "^2.6.7", "pako": "^2.0.3", "react": "^16.13.1", "react-dom": "^16.13.1", "react-infinite-scroll-component": "^6.1.0", "react-router-dom": "^5.2.0", "shrink-ray-current": "^4.1.2", "snowpack": "^2.6.4", "sql-formatter": "^2.3.3", "sql-template-strings": "^2.2.2", "terser": "^4.8.0", "uuidv4": "^6.2.0", "version-bump-prompt": "^6.1.0", "ws": "^7.4.6" }, "snowpack": { "exclude": [ "*/" ], "install": [ "react", "react-dom", "htm/react", "react-router-dom", "chart.js", "chartjs-plugin-labels", "uuidv4", "pako", "react-infinite-scroll-component" ], "installOptions": { "dest": "./ui/web_modules", "sourceMap": false, "env": { "NODE_ENV": "production" }, "alias": { "chart.js": "chart.js" } } } }

I want to upgrade my snowpack version to 3.8.7 but I am not able to configure the install directory and I have done following changes: { "name": "web", "version": "3.2.29", "description": "CCUC Web micro-service", "private": true, "type": "module", "scripts": { "start": "cd server && node --es-module-specifier-resolution=node index.js", "bump": "bump", "watch": "cd server && nodemon --experimental-modules --es-module-specifier-resolution=node index.js", "test": "npm run test:ui && npm run test:server", "lint": "npm run eslint && npm run stylelint", "prepare": "snowpack build && sh ./scripts/terser.sh", "test:server": "cd server && jest --coverage --ci -c ../jest.server.config.json", "test:ui": "sh scripts/webtocommon.sh && jest -u --coverage --ci -c jest.ui.config.json && sh scripts/commontoweb.sh", "updatesnapshot:ui": "sh webtocommon.sh && jest --updateSnapshot --coverage --ci -c jest.ui.config.json && sh commontoweb.sh", "stylelint": "stylelint --fix --config .stylelintrc.json ui/asset/css", "eslint": "eslint -c .eslintrc.json .", "eslint:jenkins": "eslint -c .eslintrc.json . -f checkstyle > eslint.xml", "docker": "npm run prepare && docker build --compress -t web .", "eslint:server": "eslint -c .eslintrc.json ./server -f checkstyle > ./server/eslint.xml", "eslint:ui": "eslint -c .eslintrc.json ./ui -f checkstyle > ./ui/eslint.xml" }, "keywords": [], "author": "GG Team", "license": "UNLICENSED", "devDependencies": { "@babel/cli": "^7.7.5", "@babel/core": "^7.10.5", "@babel/plugin-proposal-class-properties": "^7.10.4", "@babel/plugin-syntax-dynamic-import": "^7.7.4", "@babel/plugin-transform-async-to-generator": "^7.10.4", "@babel/plugin-transform-runtime": "^7.10.5", "@babel/preset-env": "^7.10.4", "@babel/preset-react": "^7.10.4", "@testing-library/jest-dom": "^5.11.1", "babel-jest": "^26.1.0", "babel-plugin-minify-builtins": "^0.5.0", "babel-plugin-minify-dead-code-elimination": "^0.5.1", "babel-polyfill": "^6.0.16", "babel-preset-minify": "^0.5.1", "browser-resolve": "^1.11.3", "chai": "^4.2.0", "enzyme": "^3.11.0", "enzyme-adapter-react-16": "^1.15.2", "enzyme-to-json": "^3.5.0", "eslint": "^7.5.0", "eslint-config-standard": "^14.1.0", "eslint-plugin-import": "^2.22.0", "eslint-plugin-node": "^11.1.0", "eslint-plugin-promise": "^4.2.1", "eslint-plugin-react": "^7.20.3", "eslint-plugin-react-hooks": "^4.0.6", "eslint-plugin-standard": "^4.0.1", "jest": "^26.1.0", "jest-canvas-mock": "^2.2.0", "jest-enzyme": "^7.1.2", "jest-junit": "^11.0.1", "jsdom": "^16.3.0", "make-dir-cli": "^2.0.0", "nodemon": "^2.0.4", "openapi3-generator": "^0.13.0", "react-test-renderer": "^16.13.1", "snowpack": "^3.8.7", "stylelint": "^13.5.0", "stylelint-config-standard": "^20.0.0", "supertest": "^4.0.2", "terser": "^4.8.0" }, "dependencies": { "aws-sdk": "^2.683.0", "babel-eslint": "^10.1.0", "chart.js": "^2.9.4", "chartjs-plugin-labels": "^1.1.0", "express": "^4.17.1", "htm": "^3.0.4", "kafkajs": "^1.15.0", "mathjs": "^7.2.0", "mysql2": "^2.3.2", "node-fetch": "^2.6.7", "pako": "^2.0.3", "react": "^16.13.1", "react-dom": "^16.13.1", "react-infinite-scroll-component": "^6.1.0", "react-router-dom": "^5.2.0", "shrink-ray-current": "^4.1.2", "snowpack": "^3.8.7", "sql-formatter": "^2.3.3", "sql-template-strings": "^2.2.2", "terser": "^4.8.0", "uuidv4": "^6.2.0", "version-bump-prompt": "^6.1.0", "ws": "^7.4.6" }, "snowpack": { "exclude": [ "*/" ],

"packageOptions": {
  "knownEntrypoints": [
    "react",
    "react-dom",
    "htm/react",
    "react-router-dom",
    "chart.js",
    "chartjs-plugin-labels",
    "uuidv4",
    "pako",
    "react-infinite-scroll-component"
  ],
  "dest": "./ui/web_modules",
  "sourceMap": false,
  "env": {
    "NODE_ENV": "production"
  },
  "alias": {
    "chart.js": "chart.js"
  }
}

} }

FredKSchott / snowpack

Support custom directory for installed dependencies #441