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

feat: update to lit v2 #36

Merged
merged 9 commits into from
Aug 17, 2021
Merged

feat: update to lit v2 #36

merged 9 commits into from
Aug 17, 2021

Conversation

LarsDenBakker
Copy link
Member

What I did

  1. Updated to lit v2
  2. Simplified build config
  3. Dropped support for IE11
  4. Fixed sorting of packages.json
  5. Sort dependencies

@LarsDenBakker LarsDenBakker force-pushed the feat/lit-v2 branch 3 times, most recently from d27e3e3 to 9372e85 Compare June 13, 2021 11:27
Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Bunch of tiny nits and a question or two...

@bennypowers
Copy link
Member

bennypowers commented Aug 17, 2021

I tried resolving conflicts on my local copy of the branch by taking both changes (and manually resolving duplicate dependencies as later version).

yarn && yarn test
UNKNOWN_OPTION: Unknown option: --scaffoldFilesFor
Error: Command failed: node -r @babel/register /Users/bennyp/Developer/open-wc/create/src/create.js       --destinationPath /Users/bennyp/Developer/open-wc/create/test/snapshots       --type scaffold       --scaffoldType app       --features linting testing demoing building       --scaffoldFilesFor testing demoing building       --typescript false       --tagName scaffold-app       --writeToDisk true       --installDependencies false

/Users/bennyp/Developer/open-wc/create/node_modules/command-line-args/dist/index.js:1377
        throw err

@bennypowers
Copy link
Member

Alright I got the tests to run but that raised some questions. This PR, as it is when master is merged in, seems like it's in an odd state. There are multiple references to scaffoldFilesFor, but the main cli entrypoint removed the option. I'm not sure how to proceed. Do we / did we remove that option entirely?

Merges in master
renames (now internal) scaffoldFilesFor to _scaffoldFilesFor
removes deprecated CLIEngine usage
installs devDependencies to match fully loaded project
@bennypowers
Copy link
Member

bennypowers commented Aug 17, 2021

This PR installs some seemingly useless devDependencies:

    "@open-wc/testing": "^2.5.33",
    "@rollup/plugin-babel": "^5.3.0",
    "@web/rollup-plugin-html": "^1.9.1",
    "@web/rollup-plugin-import-meta-assets": "^1.0.7",

and

    "lit": "^2.0.0-rc.2",

These are here because our integration testing writes actual files to the /create package root, therefore eslint import plugin is able to resolve them in /create/node_modules. This is not ideal. Tests which rely on the scaffolded package's node_modules should run e2e in an isolated temp dir that's not affected by create's node_modules, or in a virtual filesystem where deps are stubbed.

@bennypowers
Copy link
Member

replaced @open-wc/eslint-config and eslint-config-prettier with @open-wc and prettier, respectively, in eslint-config "extends" blocks

@bennypowers bennypowers merged commit fc090cd into master Aug 17, 2021
@bennypowers bennypowers deleted the feat/lit-v2 branch August 17, 2021 12:44
@bennypowers bennypowers mentioned this pull request Aug 17, 2021
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.

3 participants