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

Shield Libary: generate three output formats for broad compatibility #905

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

zekefarwell
Copy link
Collaborator

@zekefarwell zekefarwell commented Jul 25, 2023

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 zekefarwell changed the title Zeke shieldlib iife 890 Shield Llbary: generate three output formats for broad compatibility Jul 25, 2023
@zekefarwell zekefarwell changed the title Shield Llbary: generate three output formats for broad compatibility Shield Libary: generate three output formats for broad compatibility Jul 25, 2023
…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.
@github-actions
Copy link

PR Preview:

Sprite Sheets:

Sprites 1x
Sprites 2x

…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.
@zekefarwell
Copy link
Collaborator Author

zekefarwell commented Jul 25, 2023

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

@1ec5
Copy link
Collaborator

1ec5 commented Jul 26, 2023

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.

createGraphics(bounds: Bounds): CanvasRenderingContext2D {

@ZeLonewolf
Copy link
Owner

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 ZeLonewolf mentioned this pull request Jul 30, 2023
@ZeLonewolf
Copy link
Owner

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?

@1ec5
Copy link
Collaborator

1ec5 commented Jul 31, 2023

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

@ZeLonewolf
Copy link
Owner

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

@1ec5
Copy link
Collaborator

1ec5 commented Jul 31, 2023

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.

@ZeLonewolf
Copy link
Owner

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
Copy link
Owner

Fix in #909.

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

@ZeLonewolf
Copy link
Owner

Does this PR conflict with #900 at all?

@zekefarwell
Copy link
Collaborator Author

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

@ZeLonewolf
Copy link
Owner

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?

@zekefarwell
Copy link
Collaborator Author

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

@ZeLonewolf
Copy link
Owner

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.

@claysmalley claysmalley added the enhancement New feature or request label Sep 4, 2023
@ZeLonewolf
Copy link
Owner

The addition of:

"transpileOnly": true

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

@claysmalley claysmalley added shield-generator Issues specific to the shield library and removed shields labels May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request shield-generator Issues specific to the shield library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants