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

Set output directory to dist #30

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Set output directory to dist #30

wants to merge 2 commits into from

Conversation

fvilers
Copy link

@fvilers fvilers commented Aug 2, 2020

tony added a commit to tony/.dot-config that referenced this pull request Mar 13, 2021
@tony
Copy link

tony commented Mar 13, 2021

I like this change. Looks good to me.

QA: I cherry-picked this into a personal branch at tony#1

@tony
Copy link

tony commented Mar 13, 2021

@arthurevans Thoughts on merging this? This cleans up output and simplifies the build

One thing I haven't considered yet is the production build / packaging (via https://github.com/PolymerLabs/lit-element-starter-ts/issues/14#issuecomment-614293152)

Would this do the trick?

diff --git a/package.json b/package.json
index 570393a..bdda163 100644
--- a/package.json
+++ b/package.json
@@ -2,8 +2,9 @@
   "name": "lit-element-starter-ts",
   "version": "0.0.0",
   "description": "A simple web component",
-  "main": "my-element.js",
-  "module": "my-element.js",
+  "main": "./dist/my-element.js",
+  "module": "./dist/my-element.js",
+  "types": "./dist/my-element.d.ts",
   "type": "module",
   "scripts": {
     "build": "tsc",

(I suppose with lit-next also in motion at https://github.com/Polymer/lit-html/tree/lit-next/packages/lit-starter-ts this will be superseded, but in the mean time this is nice!)

tony added a commit to tony/lit-element-starter-ts that referenced this pull request Mar 13, 2021
@tony
Copy link

tony commented Mar 13, 2021

@fvilers Some more places to update:

diff --git a/dev/index.html b/dev/index.html
index 3d6ce88..f2659e3 100644
--- a/dev/index.html
+++ b/dev/index.html
@@ -4,7 +4,7 @@
   <head>
     <meta charset="utf-8" />
     <title>&lt;o-embed> Demo</title>
-    <script type="module" src="../o-embed.js"></script>
+    <script type="module" src="../dist/o-embed.js"></script>
     <style>
       p {
         border: solid 1px blue;
diff --git a/package.json b/package.json
index a6c745a..eadfc46 100644
--- a/package.json
+++ b/package.json
@@ -27,7 +27,7 @@
     "test:watch": "karma start karma.conf.cjs --auto-watch=true --single-run=false",
     "test:update-snapshots": "karma start karma.conf.cjs --update-snapshots",
     "test:prune-snapshots": "karma start karma.conf.cjs --prune-snapshots",
-    "checksize": "rollup -c ; cat o-embed.bundled.js | gzip -9 | wc -c ; rm o-embed.bundled.js"
+    "checksize": "rollup -c ; cat dist/o-embed.bundled.js | gzip -9 | wc -c ; rm dist/o-embed.bundled.js"
   },
   "keywords": [
     "web-components",
diff --git a/rollup.config.js b/rollup.config.js
index f17c504..ca0cf21 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -20,7 +20,7 @@ import {terser} from 'rollup-plugin-terser';
 export default {
   input: 'dist/o-embed.js',
   output: {
-    file: 'o-embed.bundled.js',
+    file: 'dist/o-embed.bundled.js',
     format: 'esm',
   },
   onwarn(warning) {

tony added a commit to tony/lit-element-starter-ts that referenced this pull request Mar 13, 2021
@tony
Copy link

tony commented Mar 13, 2021

@arthurevans
Copy link
Contributor

Sorry this didn't get reviewed earlier. I don't think I'm really the person to make this call.

@kevinpschaaf was updating the lit-next version and may have thoughts.

@arthurevans
Copy link
Contributor

FWIW I agree it makes the whole thing cleaner. If I recall correctly, we originally didn't have a dist or equivalent directory because we wanted to keep the TS output looking as much like the JS version of the project as possible. But perhaps @justinfagnani would remember better than I.

@tony
Copy link

tony commented Mar 14, 2021

we wanted to keep the TS output looking as much like the JS version of the project as possible

Noted! What if the JS version has this modification made, as well?

In hindsight, this PR is still fine with me on a fork I made to https://github.com/tony/oembed-component.

The changes to output to dist/ didn't seem to break anything. It still leaves the user to setup their bundling/packaging/minification.

The ref for the template I used is at https://github.com/tony/lit-element-starter-ts/tree/tony (I also updated the packages)

@fvilers
Copy link
Author

fvilers commented Mar 15, 2021

HI @tony, thanks for your review. I've updated the PR.

@callumlocke
Copy link

Is this going to be merged at any point? The way it currently works (without a dedicated output folder for build) is unfamiliar and unhelpful. It requires a lot of undocumented configuration changes in various places just to add a new custom element or rename the built-in my-element one.

@melMass melMass mentioned this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants