keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
391 stars 109 forks source link

chore(common): ldml-keyboard-xml-reader uses `import.meta.url` but we need to be passing paths from caller #10358

Closed mcdurdin closed 2 months ago

mcdurdin commented 9 months ago

The following should not be compiled in:

https://github.com/keymanapp/keyman/blob/5dbda001fcd8973b4922292b5c0731748380a263/common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts#L24-L26

The imports path cannot have a default because we cannot rely on the files being available from the package anyway, and as we can see, some build environments can't use it anyway.

This will be less critical with the split of common/web/types to move developer source file types into a developer package (#9665), but as a matter of policy and consistency we need to do it anyway.

[web/src/app/browser] ## build starting...
▲ [WARNING] "import.meta" is not available in the configured target environment ("es2015") and will be empty [empty-import-meta]

    ../../../../common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts:25:26:
      25 │     return ['../import/', import.meta.url];
         ╵                           ~~~~~~~~~~~

▲ [WARNING] "import.meta" is not available in the configured target environment ("es2015") and will be empty [empty-import-meta]

    ../../../../common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts:25:26:
      25 │     return ['../import/', import.meta.url];
         ╵                           ~~~~~~~~~~~

▲ [WARNING] "import.meta" is not available in the configured target environment ("es2015") and will be empty [empty-import-meta]

    ../../../../common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts:25:26:
      25 │     return ['../import/', import.meta.url];
         ╵                           ~~~~~~~~~~~

▲ [WARNING] "import.meta" is not available in the configured target environment ("es2015") and will be empty [empty-import-meta]

    ../../../../common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts:25:26:
      25 │     return ['../import/', import.meta.url];
         ╵                           ~~~~~~~~~~~
srl295 commented 3 months ago

@mcdurdin so, the callers (two tests, and two sites within kmc) should each have their own definition of this, and remove this function? (maybe combine the two kmc callers into one).

common/web/types/test/helpers/reader-callback-test.ts:
  11  const readerOptions: LDMLKeyboardXMLSourceFileReaderOptions = {
  12:   importsPath: fileURLToPath(new URL(...LDMLKeyboardXMLSourceFileReader.defaultImportsURL))
  13  };

developer/src/kmc/src/commands/buildClasses/BuildLdmlKeyboard.ts:
  13      const ldmlCompilerOptions: kmcLdml.LdmlCompilerOptions = {...options, readerOptions: {
  14:       importsPath: fileURLToPath(new URL(...LDMLKeyboardXMLSourceFileReader.defaultImportsURL))
  15      }};

developer/src/kmc/src/commands/buildTestData/index.ts:
  19      readerOptions: {
  20:       importsPath: fileURLToPath(new URL(...LDMLKeyboardXMLSourceFileReader.defaultImportsURL))
  21      }

developer/src/kmc-ldml/test/helpers/index.ts:
  39    readerOptions: {
  40:     importsPath: fileURLToPath(new URL(...LDMLKeyboardXMLSourceFileReader.defaultImportsURL))
  41    }
mcdurdin commented 3 months ago

so, the callers (two tests, and two sites within kmc) should each have their own definition of this, and remove this function? (maybe combine the two kmc callers into one).

Yep, that sounds about right

srl295 commented 2 months ago

👉 start this on top of the 🐉 chain @ #12101

sentry-io[bot] commented 2 months ago

Sentry Issue: KEYMAN-DEVELOPER-251

srl295 commented 2 months ago

per https://github.com/keymanapp/keyman/pull/12108#issuecomment-2272549431 closing this