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

Expose strict mode functionality #41

Merged
merged 4 commits into from
Apr 11, 2019

Conversation

lukechilds
Copy link
Member

@lukechilds lukechilds commented Apr 8, 2019

This PR adds support for using assert in strict mode.

Resolves: #37
Makes some progress towards: #32

Ported from this Node.js PR: nodejs/node#17002

Tests are all failing. This isn't due to any changes in this PR they are already failing in master: #33

I've tested locally on a couple of different Node.js versions and tests are passing but would feel a lot more confident getting this properly tested against all browser targets.

Happy to look into the Travis issue and submit a separate PR resolving that if you want.

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

Great thanks, just one compatibility comment :)

assert.js Outdated Show resolved Hide resolved
@lukechilds
Copy link
Member Author

lukechilds commented Apr 9, 2019

@goto-bus-stop addressed feedback, let me know if requireing the ponyfill is ok.

Also resolved some of the issues with the tests here: #42

@goto-bus-stop
Copy link
Member

I think the buffer shim is inlined because recent buffer versions don't work at all on old IE (throws an error on load). object-assign should be ok, it seems like assert only supported IE9+ already… i'll have a closer look later to confirm what the current oldest supported version is and document it somewhere :P

@lukechilds
Copy link
Member Author

I think the buffer shim is inlined because recent buffer versions don't work at all on old IE

Ahhhh, that makes sense.

i'll have a closer look later to confirm what the current oldest supported version is and document it somewhere :P

That would be great, thanks!

@lukechilds
Copy link
Member Author

@goto-bus-stop is this ok to be merged?

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

yes, turns out this module already only works in IE9+ because of Object.getPrototypeOf. thanks!

@lukechilds lukechilds merged commit 8002bc1 into browserify:master Apr 11, 2019
@lukechilds
Copy link
Member Author

lukechilds commented Apr 11, 2019

@goto-bus-stop should we publish a new release with this?

I don't have npm permissions.

@goto-bus-stop
Copy link
Member

ideally yes! i don't have publish perms either :'

@defunctzombie do you have a moment to give the browserify org members publish access on npm?

npm access grant read-write browserify:developers assert

@lukechilds
Copy link
Member Author

lukechilds commented Apr 11, 2019

(or @calvinmetcalf or @solderjs) 🙏

@lukechilds
Copy link
Member Author

Any update on this?

Ping @defunctzombie @calvinmetcalf @solderjs

@goto-bus-stop
Copy link
Member

📦 1.5.0 🎉

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.

Strict mode?
2 participants