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

Try to fix windows build #20

Merged
merged 15 commits into from
Mar 12, 2020
Merged

Try to fix windows build #20

merged 15 commits into from
Mar 12, 2020

Conversation

flaambe
Copy link
Contributor

@flaambe flaambe commented Feb 28, 2020

Tests to fix:

  • Acceptance
  • Class
  • Maps
  • Promises
  • Sets
  • Typed arrays
  • Weaksets

@keithamus
Copy link
Member

Looks like destructuring is also failing. Perhaps bringing in @babel/preset-env and setting it to IE11 would get allow for all the needed features to be transpiled.

@flaambe
Copy link
Contributor Author

flaambe commented Feb 28, 2020

Object doesn't support property or method 'entries' I think we need core-js

3 fails left to fix

I think the last problem is Object.getOwnPropertySymbols not properly polyfilled

Symbol('foo') turns to Symbol{}

@keithamus

@keithamus keithamus changed the base branch from use-github-actions to master March 2, 2020 11:42
index.js Outdated Show resolved Hide resolved
lib/date.js Show resolved Hide resolved
@flaambe
Copy link
Contributor Author

flaambe commented Mar 2, 2020

What is better to add a babel plugin to transform

 Object.entries, Object.assign

or write polyfill manually?

@keithamus
Copy link
Member

We seem to only use Object.entries and Object.assign in the 1 table in ./test/object.js. I'd suggest we-writing them completely; so L3-6 of ./test/objects.js should become:

const table = {
  objects: loupe,
  'objects (Object.create(null))': (obj, ...rest) => {
    const newObj = Object.create(null)
    for(const k in obj) {
      Object.defineProperty(newObj, k, Object.getOwnPropertyDescriptor(obj, k)
    }
    return loupe(newObj, ...rest)
  }
}
for (const suite of table) {
  const inspect = table[suite]

@flaambe
Copy link
Contributor Author

flaambe commented Mar 3, 2020

TypeError: table[Symbol.iterator] is not a function @keithamus

If i changle like this, than it works

for (const suite of Array.from(table)) {
  const inspect = table[suite]

Or just

for (const suite in table) {
  const inspect = table[suite]

Sorry, I accidentally deleted your commit, can you push it again?

@keithamus
Copy link
Member

Yeah sorry I meant for(const suite in.... Sorry I can't give this my full attention.

I've pushed the changes. 🤞 this build passes! Thanks for all your awesome efforts here @flaambe!

@flaambe
Copy link
Contributor Author

flaambe commented Mar 3, 2020

I think it will be difficult to fix everything without polyfills (Set, WeakSet, Promises and etc). What do you think about it? @keithamus

@keithamus
Copy link
Member

Sadly our guideline is to support IE11 until EOL: https://github.com/chaijs/guidelines/blob/master/support-cycle.md#internet-explorer

We can load polyfills in the local test harness, but the library must run without them, as we cannot mutate the global environment within user's test harnesses. They might want to bring in alternative polyfills.

@flaambe
Copy link
Contributor Author

flaambe commented Mar 3, 2020

Ok, thanks for explaining

lib/regexp.js Outdated Show resolved Hide resolved
@keithamus
Copy link
Member

@flaambe I just pushed up a new commit. If you want to continue force pushing to this branch it might be worth using --force-with-lease - if you're not already. It prevents you from force pushing a branch to a stale origin.

@flaambe
Copy link
Contributor Author

flaambe commented Mar 4, 2020

@keithamus Ok

I think the problem is that IE map,sets do not accept arrays

@flaambe
Copy link
Contributor Author

flaambe commented Mar 5, 2020

It's strange, the documentation says that the name property of typed arrays is supported starting from IE 10

@flaambe
Copy link
Contributor Author

flaambe commented Mar 10, 2020

Maybe just skip tests where the IE doesn't support Promises, Weakset and iterable parameter for Map, Set? @keithamus

@keithamus
Copy link
Member

Tests must consistently pass for IE11 as they do in other browsers. If IE11 has different results when people come to depend on them that will not be ideal. We should instead polyfill the test environment to ensure these pieces are there.

@keithamus
Copy link
Member

keithamus commented Mar 10, 2020

Looks like the only thing failing now is Symbol because of Corejs. It polyfills Symbol.prototype.description - so we could write code to detect for that and use it over toString().

--- lib/symbol.js
+++ lib/symbol.js
@@ -1,3 +1,6 @@
 export default function inspectSymbol(value) {
+  if ('description' in Symbol.prototype) {
+    return `Symbol(${value.description})`
+  }
   return value.toString()
 }

@flaambe
Copy link
Contributor Author

flaambe commented Mar 10, 2020 via email

@flaambe
Copy link
Contributor Author

flaambe commented Mar 10, 2020

As I said above @keithamus

Symbol('foo') turns to Symbol{}

@keithamus
Copy link
Member

Yeah, I see that. Not sure where that is coming from.

@flaambe
Copy link
Contributor Author

flaambe commented Mar 10, 2020

I think I should rebase, since there are a lot of unnecessary commits, but first we need to decide what to do with Symbol

@flaambe
Copy link
Contributor Author

flaambe commented Mar 11, 2020

I think this is a problem, because of the bundle Rollup + Babel. Maybe just skip these 3 tests for now, as I don’t see a solution? @keithamus

flaambe and others added 5 commits March 12, 2020 11:48
-drop karma-detect-browsers
-target IE11 with preset-env
-use corejs
-number and description error keys added
-use dateObject directly
@keithamus
Copy link
Member

I've got a good feeling about this one @flaambe

@keithamus keithamus merged commit 1e52410 into chaijs:master Mar 12, 2020
@keithamus
Copy link
Member

🎉 congratulations @flaambe! You did it! Thank you so much for all your work on this 👍

@flaambe
Copy link
Contributor Author

flaambe commented Mar 12, 2020

Thank you for all the advice @keithamus

@flaambe flaambe deleted the fix-windows-build branch March 12, 2020 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants