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

fix: npm install with npm 7 #592

Merged
merged 1 commit into from
Mar 16, 2021
Merged

fix: npm install with npm 7 #592

merged 1 commit into from
Mar 16, 2021

Conversation

Haprog
Copy link
Contributor

@Haprog Haprog commented Mar 16, 2021

npm install was failing with npm 7 unless using the --legacy-peer-deps
flag. Remove dependency to old version of eslint-config-vaadin
and stylelint-config-vaadin while adapting the configs to try to keep
the rules mostly the same for now. We might consider using the latest
version of eslint-config-vaadin and stylelint-config-vaadin again when
we migrate away from bower and HTML Imports and maybe migrate
this repo to use TypeScript.

The problem was old versions of eslint and stylelint declared as peer dependencies of eslint-config-vaadin and stylelint-config-vaadin and we can't yet use newer versions of eslint-config-vaadin and stylelint-config-vaadin because they have changed to support LitElement and TypeScript based projects and those configs don't work as is for vaadin-router which is still based on HTML Imports and doesn't use TS.

Fixes #541.

npm install was failing with npm 7 unless using the --legacy-peer-deps
flag. Remove dependency to old version of eslint-config-vaadin
and stylelint-config-vaadin while adapting the configs to try to keep
the rules mostly the same for now. We might consider using the latest
version of eslint-config-vaadin and stylelint-config-vaadin again when
we migrate away from bower and HTML Imports and maybe migrate
this repo to use TypeScript.

Fixes #541.
@Haprog Haprog added the hilla label Mar 16, 2021
@@ -222,7 +222,7 @@ class Resolver {
? this.constructor.__createUrl(
this.baseUrl,
document.baseURI || document.URL
).href.replace(/[^\/]*$/, '')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an unnecessary escape (complained by linter rule). Most characters don't need to be escaped inside a character group [].

@@ -172,7 +172,7 @@ export class Router extends Resolver {
const baseHref = baseElement && baseElement.getAttribute('href');
super([], Object.assign({
// Default options
baseUrl: baseHref && Resolver.__createUrl(baseHref, document.URL).href.replace(/[^\/]*$/, '')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an unnecessary escape (complained by linter rule). Most characters don't need to be escaped inside a character group [].

Copy link
Contributor

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

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

Tried locally with npm7, works for me

@haijian-vaadin haijian-vaadin merged commit e740512 into master Mar 16, 2021
@Haprog Haprog deleted the haprog/fix-npm7 branch March 31, 2021 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vaadin-router doesn't run in npm7 (node 15)
2 participants