-
Notifications
You must be signed in to change notification settings - Fork 65
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
Shield Libary: generate three output formats for broad compatibility #905
base: main
Are you sure you want to change the base?
Conversation
…ility Specifying the `main`, `module`, and `browser` options in package.json should allow the proper format to be selected by the importing context.
…module resolution issue ts-node is importing the common js module build rather than the esmodule build, but treating it as an esmodule. This leads to name resolution errors. importing the typescript source file instead of the build output works around this issue.
PR Preview: Sprite Sheets: |
…e resolution issue ts-node is importing the common js module build rather than the esmodule build, but treating it as an esmodule. This leads to name resolution errors. importing the typescript source file instead of the build output works around this issue.
The builds were failing originally due an import name resolution issue. I worked around this with the same hack already in use here. I imagine there is probably a better way, but I'm a bit out of my depth on this. Unfortunately the Ubuntu test build is still failing due to some other issue that I don't understand. I'm actually getting the same failure when I run the tests locally on my Mac (Apple Silicon), but the MacOS build here is successfully. 🤷 |
I wonder if the import name resolution issue is caused by this use of a DOM type as an instance method’s return value. In the absence of an explicit import, it would be coming from the
|
I re-ran the test build several times in CI and it errored on the same test case each time. The test failure is bizarre because it's erroring on one specific test case in the middle. |
The issue is at: expectGloss("en", "L’Aquila", "L'Aquila", "L’Aquila", "L'Aquila"); Is it possible that we're seeing some kind of difference in unicode diacritic processing between javascript versions that could be causing this? |
That would be surprising to me, since This test case was added in response to #592 (comment). Preferring OSM’s straight apostrophe over Wikidata’s curly apostrophe is suboptimal from a typographical standpoint but not incorrect linguistically. We shouldn’t insist on this behavior everywhere, but if it’s happening on some platforms, I guess it’s OK and we can replace this test case with something we feel more strongly about. |
I'm exploring the root of this issue on the L'Aquila test case over in #908. If I can isolate the issue, what I would plan to do is to restructure the test cases such that the expected test result matches the node-version-specific maplibre expression behavior. Right now we are hard-coding the epected label and gloss, but we should be able to generate the expected result in code and verify that the expression that we've built does the same thing as the javascript code (which in theory is using the same javascript functions to parse labels). |
This is a good thing, in my opinion. We need to know when the expression yields the wrong end-user result in some configuration, regardless of whether it arrived at that result via the “right” JavaScript functions. |
If maplibre works differently in different node environments (not proven yet), I see no other way to handle testing. |
Fix in #909. Problem is upstream in node on ubuntu, so let's just not use that version. |
Does this PR conflict with #900 at all? |
I don't think it should, but might as well merge it in and then update this branch to know for sure. |
Dumb question, how do the three output artifacts get made available to end-users? Does |
I don't know the answer to this question and that's one of the reasons this PR is still a draft. It looks like it is up to a library author to configure which files end up getting published or not. |
I think for github CI artifacts, we can use https://github.com/actions/upload-pages-artifact and just add a section to the ubuntu builder that points to the output jar files. Probably easiest if the output jar files post to a dedicated folder that we can point the upload-artifacts action to. |
The addition of: "transpileOnly": true Is what seems to have broken this build, based on a diff inspection. |
Building on #901, this PR updates the esbuild script to generate all three possible output formats and references them in the package.json main, module, and browser fields. This should allow the library to be used in a variety of different contexts that may or may not support ES Modules.
I am opening this as a draft PR for now since it may make sense to include some more related changes before merging. PR previews here would let them be separately tested on a live URL.
The shield library is currently being built twice. Once in the library build script, and once in the main americana build script. I think the one in the main build script is vestigial from before the shield library got split out. However, when I removed it, the live reload feature threw an error. Looks like it somehow expects a
shieldLibContext
from the build script.I don't currently have time to figure out how either of these things work in order to make those changes. If anyone else wants to add on to this PR, go right ahead. Or if the consensus is just to move ahead without them that's fine too.