openstreetmap-americana icon indicating copy to clipboard operation
openstreetmap-americana copied to clipboard

Shield Libary: generate three output formats for broad compatibility

Open zekefarwell opened this issue 1 year ago • 16 comments

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.

zekefarwell avatar Jul 25 '23 15:07 zekefarwell

PR Preview:

Sprite Sheets:

Sprites 1x Sprites 2x

github-actions[bot] avatar Jul 25 '23 16:07 github-actions[bot]

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. 🤷

zekefarwell avatar Jul 25 '23 16:07 zekefarwell

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 window when running in a browser.

https://github.com/ZeLonewolf/openstreetmap-americana/blob/efff6cdecf68504a02b1343ddc760327f416616f/shieldlib/src/document_graphics.ts#L13

1ec5 avatar Jul 26 '23 06:07 1ec5

I re-ran the test build several times in CI and it errored on the same test case each time. I ran the test suite locally on my Ubuntu and it built normally.

The test failure is bizarre because it's erroring on one specific test case in the middle.

ZeLonewolf avatar Jul 30 '23 02:07 ZeLonewolf

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?

ZeLonewolf avatar Jul 30 '23 02:07 ZeLonewolf

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 isn’t a diacritic at all. This is the only way I can explain the result of “L'Aquila”, but it suggests a possible standards compliance bug in the Node engine on that platform.

This test case was added in response to https://github.com/ZeLonewolf/openstreetmap-americana/pull/592#issuecomment-1332367624. 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.

1ec5 avatar Jul 31 '23 07:07 1ec5

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).

ZeLonewolf avatar Jul 31 '23 14:07 ZeLonewolf

Right now we are hard-coding the epected label and gloss

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.

1ec5 avatar Jul 31 '23 17:07 1ec5

Right now we are hard-coding the epected label and gloss

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.

ZeLonewolf avatar Jul 31 '23 18:07 ZeLonewolf

Fix in #909.

Problem is upstream in node on ubuntu, so let's just not use that version.

ZeLonewolf avatar Jul 31 '23 23:07 ZeLonewolf

Does this PR conflict with #900 at all?

ZeLonewolf avatar Aug 01 '23 02:08 ZeLonewolf

I don't think it should, but might as well merge it in and then update this branch to know for sure.

zekefarwell avatar Aug 01 '23 02:08 zekefarwell

Dumb question, how do the three output artifacts get made available to end-users? Does npm publish just take care of that or do we need to designate artifacts in the github CI?

ZeLonewolf avatar Aug 01 '23 17:08 ZeLonewolf

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. https://stackoverflow.com/questions/31642477/how-can-i-publish-an-npm-package-with-distribution-files

zekefarwell avatar Aug 01 '23 19:08 zekefarwell

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.

ZeLonewolf avatar Aug 02 '23 17:08 ZeLonewolf

The addition of:

"transpileOnly": true

Is what seems to have broken this build, based on a diff inspection.

ZeLonewolf avatar May 10 '24 12:05 ZeLonewolf