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

Feature/vite #2

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

Feature/vite #2

wants to merge 8 commits into from

Conversation

lamtev178
Copy link
Collaborator

No description provided.

package.json Outdated
"plugin"
],
"scripts": {
"test": "cd ./test && webpack"
"test": "cd ./test && npm install && npm run dev"
Copy link
Member

Choose a reason for hiding this comment

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

npm i тут лишний

Copy link
Member

Choose a reason for hiding this comment

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

А, ну не лишний в текущей реализации)
Но я бы убил второй package.json


data.headTags = [
...data.headTags.slice(0, targetIndex),
Copy link
Member

Choose a reason for hiding this comment

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

Видно, что мы вставляли скрипт перед тегом <base> (если он уже был), вроде как это не просто так

@santoslos не помнишь случайно?)

Copy link
Member

Choose a reason for hiding this comment

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

Ой, сразу после , перепутал работу slice
Перед base нельзя, не найдёт его в дереве же)

index.js Outdated
if (!data.plugin.options.base) {
logger.warn('You didn\'t specify "base" field in html-webpack-plugin');
}
document.head.append(base)
Copy link
Member

Choose a reason for hiding this comment

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

Ты проверял, запросы точно после этого идут верно?
Я думаю так неправильно, т.к. у тебя <base> добавится в конец
И если по пути были ресурсы, или какой-то скрипт, то сначала оно всё выполнится
И только потом уже твой <base> поменяется

Видимо поэтому мы скрипт прямо перед <base> вставляли

(function () {
var publicPaths = <%= JSON.stringify(publicPaths || []) %>;
var fallbackBaseHref = <%= fallbackBaseHref ? `'${fallbackBaseHref}'` : 'undefined' %>;
if (document.querySelector('base') === null)
Copy link
Member

Choose a reason for hiding this comment

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

Единственное, не могу понять, почему мы были уверены, что <base> есть
Видимо его htmlwebpackplugin всегда накидывал

Copy link
Member

Choose a reason for hiding this comment

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

Получается надо самим <base> добавлять (если его нет)
Просто не через append
И в этот <base> сразу фигачить fallbackPath, как это делает htmlwebpackplugin

Copy link
Member

@crutch12 crutch12 May 26, 2023

Choose a reason for hiding this comment

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

Ну и <base> пихать в начало конеш (на стадии билда)

README.md Outdated

Extension for [html-webpack-plugin](https://github.com/ampedandwired/html-webpack-plugin) to programmatically insert or update [`<base href="...">`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base) tag **in runtime** depending on *window.location.pathname*.
Extension for [html-vite-plugin](https://github.com/ampedandwired/html-vite-plugin) to programmatically insert or update [`<base href="...">`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base) tag **in runtime** depending on _window.location.pathname_.
Copy link
Member

Choose a reason for hiding this comment

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

Тут обман

document.querySelector('base').href = '/'
console.log('New document.baseURI:', document.baseURI);
console.log("Initial document.baseURI:", document.baseURI);
document.querySelector("base").href = "/";
Copy link
Member

Choose a reason for hiding this comment

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

почему стиль у всех ковычек всех поменялся?
ещё местами потерялись ;

@@ -0,0 +1,12 @@
<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

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

Пример то репрезентативен? Надо такой пример, чтобы я в network увидел, что у меня запрос идёт в соответствии с base

@@ -0,0 +1,17 @@
import { defineConfig } from "vite";
import baseHrefRuntimeVitePlugin from "base-href-runtime-vite-plugin";
Copy link
Member

@crutch12 crutch12 May 26, 2023

Choose a reason for hiding this comment

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

Вот это я не понимаю, как этот импорт вообще работает
Должен же быть относительный из ../index.js

@crutch12
Copy link
Member

@kirusha123 посмотри тож, это переписанный плагин под vite

README.md Show resolved Hide resolved
index.d.ts Outdated

apply: (compiler: Compiler) => void;
}
declare interface baseHrefRuntimeVitePlugin {
Copy link
Member

Choose a reason for hiding this comment

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

Типы же с большой буквы всегда пишем

index.d.ts Outdated

apply: (compiler: Compiler) => void;
}
declare interface baseHrefRuntimeVitePlugin {
Copy link
Member

Choose a reason for hiding this comment

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

Чот я туплю видимо
Не могу понять, подсосутся ли эти типы, ты же не экспортишь ничего
Проверял, подсказывает он типы в vite.config.js?

"devDependencies": {
"html-webpack-plugin": "5.5",
"webpack": "^5.23.0",
Copy link
Member

Choose a reason for hiding this comment

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

А зачем всё перенёс в test/package.json?
Я бы оставил, как было
Это же devDeps, они не будут устанавливаться у потребителей

compiler.hooks.compilation.tap('BaseHrefRuntimeWebpackPlugin', (compilation) => {
HtmlWebpackPlugin.getHooks(compilation).alterAssetTagGroups.tapAsync('BaseHrefRuntimeWebpackPlugin', (data, callback) => {
if (!data.plugin.options.base) {
logger.warn('You didn\'t specify "base" field in html-webpack-plugin');
Copy link
Member

Choose a reason for hiding this comment

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

warning зачем потерял?

README.md Outdated
@@ -151,9 +114,9 @@ https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base#browser_compatibi

# References

https://www.npmjs.com/package/base-href-webpack-plugin
https://www.npmjs.com/package/base-href-vite-plugin
Copy link
Member

Choose a reason for hiding this comment

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

Ну это совсем ложь) И ниже тоже

Copy link
Member

Choose a reason for hiding this comment

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

@lamtev178 не поправил

index.js Outdated
},
];

if (html.includes("<base")) {
Copy link
Member

Choose a reason for hiding this comment

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

Наверное если !includes

index.js Outdated
@@ -13,32 +13,45 @@ function baseHrefRuntimeVitePlugin(options) {
var fallbackBaseHref = '${fallbackBaseHref}' ? '${fallbackBaseHref}' : 'undefined';

const base = document.createElement("base")
document.getElementById("myScript").after(base)
Copy link
Member

Choose a reason for hiding this comment

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

ты же вставляешь base ниже, зачем тут

index.js Outdated
injectTo: "head-prepend",
meta: { plugin: "base-href-runtime-webpack-plugin" },
attrs: {
id: "myScript",
Copy link
Member

Choose a reason for hiding this comment

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

ну id нам не нужен, раз ты вставляем на стадии билда

@crutch12 crutch12 self-assigned this May 29, 2023
index.d.ts Outdated
@@ -1,12 +1,8 @@
import { Compiler, WebpackPluginInstance } from 'webpack';
declare const BaseHrefRuntimeVitePlugin = (options: BaseVitePluginOptions) => any;
Copy link
Member

Choose a reason for hiding this comment

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

ну чё за двойные пробелы

index.js Outdated
)}] || [];
var fallbackBaseHref = '${fallbackBaseHref}' ? '${fallbackBaseHref}' : 'undefined';

document.getElementsByTagName("base")[0].href = publicPaths.find(
Copy link
Member

Choose a reason for hiding this comment

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

Ну ты смотри, как было написано раньше то

  1. проверялось, что тег есть
    if (document.querySelector('base') === null)

  2. чё за костыльный getElementsByTagName, когда есть православный querySelector

index.js Outdated
var fallbackBaseHref = '${fallbackBaseHref}' ? '${fallbackBaseHref}' : 'undefined';

document.getElementsByTagName("base")[0].href = publicPaths.find(
(path) => window.location.pathname.includes(path.replace(/\\/$/,""))
Copy link
Member

Choose a reason for hiding this comment

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

пробельчика после запятой не хватает...

index.js Outdated
}

return {
html: html.replace(/<base [^>]*/, `<base href='${fallbackBaseHref}'`),
Copy link
Member

Choose a reason for hiding this comment

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

href="..." двойные кавычки же у атрибутов, у тебя одинарные почему-то

README.md Outdated
@@ -145,12 +145,6 @@ All modern browsers, even IE7

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base#browser_compatibility

# References
Copy link
Member

Choose a reason for hiding this comment

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

Зачем убил рефы...

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.

2 participants