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.

Sign up for GitHub

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

Jump to bottom

Rebase upstream changes #274

Merged
merged 10 commits into from
Feb 12, 2024
Merged

Rebase upstream changes #274

merged 10 commits into from
Feb 12, 2024

Conversation

12wrigja
Copy link
Contributor

No description provided.

@12wrigja 12wrigja marked this pull request as draft January 12, 2024 23:52
ptomato and others added 4 commits January 12, 2024 16:12
@ptomato @12wrigja
DRY out subtraction in AdjustRoundedDurationDays
50f0d76 
This should make things easier to follow when these quantities are
replaced by the normalized form of time durations.

UPSTREAM_COMMIT=5e7000f9def946d17f60cb6364c1409d81c44d85
@ptomato @12wrigja
Don't use RoundInstant on a duration in AdjustRoundedDurationDays
d2b595a 
We should be using RoundDuration on a duration. (Even though the outcome
is exactly the same in this case and no observable operations are
performed.)

UPSTREAM_COMMIT=a2971b0b98e065c5d54c66cfa01d17b4e846aa88
@ptomato @12wrigja
Rename zdtRelative → zonedRelativeTo
3c2a697 
This is to match the spec and also match what we do in #2519.

UPSTREAM_COMMIT=f8fe68b4efa2b8ddde3135fce802f20497c3bbc8
@justingrant @12wrigja
Extend long-test timeout to 60s
2b30b98 
I've seen occasional Test262 failures at 30 seconds (perhaps related to GitHub capacity limitations?) so let's bump to 60s.

UPSTREAM_COMMIT=d2b6d5074b305f7561e62fa9c20cb60e9ce87b2c
@12wrigja 12wrigja force-pushed the rebase_updates_1 branch 2 times, most recently from 91a8afc to fde949d Compare January 13, 2024 00:22
justingrant
justingrant approved these changes Jan 13, 2024
View reviewed changes
Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! So happy that you're cranking through these commits. Soon we'll be all caught up! Thanks again, James!

tsconfig.json Show resolved Hide resolved
justingrant and others added 4 commits January 25, 2024 14:47
@justingrant @12wrigja
Refactor time zone identifier handling
db45550 
UPSTREAM_COMMIT=076f2871a8b91d78cb90aa0012ea55fdb4d94580
@justingrant @12wrigja
Support Node 20 and work around ICU calendar bug
263af3c 
Node 20 (ICU 73.1, actually) introduced a bug that broke some tests.
This PR ensures that our tests will continue to run after other users
update their CI to Node 20. See
https://bugs.chromium.org/p/chromium/issues/detail?id=1451943 for
the bug. https://bugs.chromium.org/p/chromium/issues/detail?id=1173158
is the root cause issue that was made worse in ICU 73.1.

The fix was to avoid using Intl.DateTimeFormat in the polyfill
for Gregorian-based calendars. A nice side effect of this is that
these calendars now run tests faster and no longer fail for dates
before 1582-10-15.

UPSTREAM_COMMIT=08d214e7d60feaecc7c9b94f992777c97c46f5ea
@jsoref @12wrigja
Spelling (#2610)
6384c31 
* spelling: nonexistent
* spelling: then

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>

UPSTREAM_COMMIT=bea08cd3b755d9c4f8df3394b2c47292047ab1cd
@ptomato @12wrigja
Simplify rounding in Duration.prototype.toString
0fc0ef8 
toString() can only round the seconds and lower units, so we do not need
to pass all the units in to RoundDuration - we can only round to seconds
increments and smaller anyway. This makes it clearer that implementations
can treat seconds as a single number if they wish.

UPSTREAM_COMMIT=4c2fcafd25e06ba82bf50e270a0f9ef252f1da55
@justingrant @12wrigja
Refactor time zone offset handling
c4d8482 
This commit refactors spec text and polyfill code for time zone offsets,
especially to split the handling of offsets in ISO strings from offsets
used as time zone identifiers.

This will help prepare for a later normative commit where time zone
identifiers are limited to minutes precision while ISO string offset
inputs and ZonedDateTime's `offset` property still support nanosecond
precision.

UPSTREAM_COMMIT=fd7583b00447d47d602de2341926e09f26dd450f
@12wrigja 12wrigja marked this pull request as ready for review January 27, 2024 01:40
@12wrigja
Copy link
Contributor Author

@justingrant I've noticed a couple backwards-compat issues with some of our tests:

  • earlier versions of Node don't have Intl.supportedValuesOf. I made some changes (see the latest commit) to the Demitasse tests to work around this - are those right?
  • It seems one of these test cases fails on earlier versions of Node because of the America/Port-au-Prince IANA identifier. Any ideas on what to do here? This doesn't repro on later versions of Node, so does it count as "too new" similar to the existing carve-out in the test, or should something else happen here? https://github.com/js-temporal/temporal-polyfill/actions/runs/7675453088/job/20921701127?pr=274

@justingrant
Copy link
Contributor

@12wrigja I'll take a look later this week. Thanks!

@justingrant
Copy link
Contributor

earlier versions of Node don't have Intl.supportedValuesOf. I made some changes (see the latest commit) to the Demitasse tests to work around this - are those right?

IMO these tests should be skipped instead of just returning success. For example, we can use something like itOrSkip function from this commit.

const itOrSkip = (id) => ((id === 'chinese' || id === 'dangi') && hasOutdatedChineseIcuData ? it.skip : it);
  • one of these test cases fails on earlier versions of Node because of the America/Port-au-Prince IANA identifier.

I think the fix would be to add this time zone as a special case in GetAvailableNamedTimeZoneIdentifier. Specifically, add this line:

    Dar_Es_Salaam: 'Dar_es_Salaam',
    Port_Of_Spain: 'Port_of_Spain',
    'Port-Au-Prince': 'Port-au-Prince',  // add this line 
    Isle_Of_Man: 'Isle_of_Man',
    Comodrivadavia: 'ComodRivadavia',

This is not Node-version-dependent... it should work in all Node versions. If not, let me know!

@12wrigja 12wrigja force-pushed the rebase_updates_1 branch 2 times, most recently from eb856b5 to 46025c2 Compare February 12, 2024 19:57
@12wrigja
Copy link
Contributor Author

Thanks Justin.

The addition to GetAvailableNamedTimeZoneIdentifier worked perfectly - thanks. I also added the itOrSkip-like function to disable the two new test-cases for older Node runtimes.

@12wrigja 12wrigja merged commit 990d51a into js-temporal:main Feb 12, 2024
19 checks passed
@12wrigja 12wrigja deleted the rebase_updates_1 branch February 12, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justingrant justingrant justingrant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@12wrigja @justingrant @ptomato @jsoref