You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@annotator.apache.org by GitBox <gi...@apache.org> on 2021/11/01 07:27:59 UTC
[GitHub] [incubator-annotator] tilgovi opened a new pull request #117: Run tests as ECMAScript modules
tilgovi opened a new pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117
Upgrade Mocha to v9, replace chai with the built-in assert module, replace optimal-select with @medv/finder, replace nyc with c8, and configure the test suite to run without transforming modules to CommonJS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi merged pull request #117: Run tests as ECMAScript modules
Posted by GitBox <gi...@apache.org>.
tilgovi merged pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on a change in pull request #117: Run tests as ECMAScript modules
Posted by GitBox <gi...@apache.org>.
Treora commented on a change in pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117#discussion_r744326905
##########
File path: packages/dom/src/css.ts
##########
@@ -20,10 +20,10 @@
* under the License.
*/
-import optimalSelect from 'optimal-select';
+import { finder } from '@medv/finder';
import type { CssSelector, Matcher } from '@apache-annotator/selector';
-import { ownerDocument } from './owner-document';
-import { toRange } from './to-range';
+import { ownerDocument } from './owner-document.js';
+import { toRange } from './to-range.js';
Review comment:
I like the explicitness of adding extensions, but it seems to break now when running `yarn run web:build` or `web:server`, with errors like:
> ERROR in ../packages/dom/src/index.ts
> Module not found: Error: Can't resolve './css.js' in '/vagrant/annotator/incubator-annotator/packages/dom/src'
Renaming all the `.js` imports to `.ts` seems to fix this for the `web:…` scripts; my editor (VSCodium) now does not understand the imports anymore though, not sure why. Also the linter now gives errors, not sure that is a step forward or back. Ideas?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #117: Run tests as ECMAScript modules
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117#discussion_r744392086
##########
File path: packages/dom/src/css.ts
##########
@@ -20,10 +20,10 @@
* under the License.
*/
-import optimalSelect from 'optimal-select';
+import { finder } from '@medv/finder';
import type { CssSelector, Matcher } from '@apache-annotator/selector';
-import { ownerDocument } from './owner-document';
-import { toRange } from './to-range';
+import { ownerDocument } from './owner-document.js';
+import { toRange } from './to-range.js';
Review comment:
This issue should be resolved now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #117: Run tests as ECMAScript modules
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117#discussion_r744361773
##########
File path: packages/dom/src/css.ts
##########
@@ -20,10 +20,10 @@
* under the License.
*/
-import optimalSelect from 'optimal-select';
+import { finder } from '@medv/finder';
import type { CssSelector, Matcher } from '@apache-annotator/selector';
-import { ownerDocument } from './owner-document';
-import { toRange } from './to-range';
+import { ownerDocument } from './owner-document.js';
+import { toRange } from './to-range.js';
Review comment:
I'm not getting linter errors locally or in CI. Using a `.ts` ending is wrong because TypeScript doesn't like it and it would require rewriting to be correct after compilation. I'll see what I can do about webpack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on a change in pull request #117: Run tests as ECMAScript modules
Posted by GitBox <gi...@apache.org>.
Treora commented on a change in pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117#discussion_r744517548
##########
File path: babel.config.js
##########
@@ -47,14 +50,25 @@ module.exports = (api) => {
// Options for the module-resolver plugin.
// Used for resolving source files during development.
const resolverOptions = {
- alias: {
- ...(DEV || TEST
- ? {
+ ...(DEV || TEST
+ ? {
+ alias: {
'^@apache-annotator/([^/]+)$': ([, name]) =>
- path.join(__dirname, 'packages', name, '/src/index.ts'),
- }
- : null),
- },
+ path.join(packagePath, name, '/src/index.ts'),
+ },
+ resolvePath(sourcePath, currentFile, opts) {
+ if (
+ currentFile.startsWith(packagePath) &&
+ currentFile.endsWith('.ts') &&
+ sourcePath.startsWith('.') &&
+ sourcePath.endsWith('.js')
+ ) {
+ return sourcePath.replace(/\.js$/, '.ts');
+ }
+ return resolvePath(sourcePath, currentFile, opts);
+ },
Review comment:
Unfortunate we’d need a custom resolver for this, but so be it. I wonder why TypeScript does not simply accept an `import '….ts';` statement, and would transpile it to the equivalent `.js` import.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on a change in pull request #117: Run tests as ECMAScript modules
Posted by GitBox <gi...@apache.org>.
Treora commented on a change in pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117#discussion_r744496151
##########
File path: babel.config.js
##########
@@ -53,8 +53,7 @@ module.exports = (api) => {
...(DEV || TEST
? {
alias: {
- '^@apache-annotator/([^/]+)$': ([, name]) =>
- path.join(packagePath, name, '/src/index.ts'),
+ '^@apache-annotator/([^/]+)$': `${packagePath}/\\1/src/index.ts`,
Review comment:
This last commit is exactly reversing the fix for Windows in https://github.com/apache/incubator-annotator/pull/105/files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #117: Run tests as ECMAScript modules
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117#discussion_r747276780
##########
File path: babel.config.js
##########
@@ -47,14 +50,25 @@ module.exports = (api) => {
// Options for the module-resolver plugin.
// Used for resolving source files during development.
const resolverOptions = {
- alias: {
- ...(DEV || TEST
- ? {
+ ...(DEV || TEST
+ ? {
+ alias: {
'^@apache-annotator/([^/]+)$': ([, name]) =>
- path.join(__dirname, 'packages', name, '/src/index.ts'),
- }
- : null),
- },
+ path.join(packagePath, name, '/src/index.ts'),
+ },
+ resolvePath(sourcePath, currentFile, opts) {
+ if (
+ currentFile.startsWith(packagePath) &&
+ currentFile.endsWith('.ts') &&
+ sourcePath.startsWith('.') &&
+ sourcePath.endsWith('.js')
+ ) {
+ return sourcePath.replace(/\.js$/, '.ts');
+ }
+ return resolvePath(sourcePath, currentFile, opts);
+ },
Review comment:
I don't recall the arguments, but I have read through the issues about it. It doesn't seem likely. They don't want to get into the business of rewriting imports. Their position is that you should write `.js`. The TypeScript resolver knows how to resolve that to equivalent `.ts` files, and maybe part of the answer is to use `eslint-import-resolver-typescript`. However, it's helpful to have Babel for our final build anyway, and `babel-plugin-module-resolver` _will_ rewrite imports for us, and `eslint-import-resolver-babel-module` can piggyback off that, so this is what I think is the best thing for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #117: Run tests as ECMAScript modules
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #117:
URL: https://github.com/apache/incubator-annotator/pull/117#discussion_r747278040
##########
File path: babel.config.js
##########
@@ -53,8 +53,7 @@ module.exports = (api) => {
...(DEV || TEST
? {
alias: {
- '^@apache-annotator/([^/]+)$': ([, name]) =>
- path.join(packagePath, name, '/src/index.ts'),
+ '^@apache-annotator/([^/]+)$': `${packagePath}/\\1/src/index.ts`,
Review comment:
Good catch. Pushed up another commit to fix that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@annotator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org